Closed Bug 1269775 Opened 4 years ago Closed 4 years ago

OdinMonkey: MIPS: Implement int32/int64 rotations

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: hev, Assigned: hev)

Details

Attachments

(1 file, 1 obsolete file)

* MacroAssembler::rotateLeft(Imm32 count, Register input, Register dest)
* MacroAssembler::rotateLeft(Register count, Register input, Register dest)
* MacroAssembler::rotateRight(Imm32 count, Register input, Register dest)
* MacroAssembler::rotateRight(Register count, Register input, Register dest)
* CodeGeneratorMIPS64::visitRotate64(LRotate64* lir)
Attachment #8748429 - Flags: review?(bbouvier)
Comment on attachment 8748429 [details] [diff] [review]
0001-Bug-1269775-OdinMonkey-MIPS-Implement-int32-int64-ro.patch

Review of attachment 8748429 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch. Int32 rotations should work well. But int64 rotations are missing some code, I think. Feel free to split the patches (one for i32 r=me, the other for i64 and ask me for review on this one).

::: js/src/jit/mips64/CodeGenerator-mips64.cpp
@@ +304,5 @@
>  }
>  
>  void
> +CodeGeneratorMIPS64::visitRotate64(LRotate64* lir)
> +{

It seems lowerForShiftInt64 is not implemented for mips64 / mips-shared, so this is technically dead code.

@@ +306,5 @@
>  void
> +CodeGeneratorMIPS64::visitRotate64(LRotate64* lir)
> +{
> +    MRotate* mir = lir->mir();
> +    Register input = ToRegister(lir->input());

Is there a constraint that input == output register? If MIPS is more well-behaved than x86, like ARM, I wouldn't think so.
Attachment #8748429 - Flags: review?(bbouvier)
Attachment #8748429 - Attachment is obsolete: true
Attachment #8748958 - Flags: review?(bbouvier)
Comment on attachment 8748958 [details] [diff] [review]
0001-Bug-1269775-OdinMonkey-MIPS-Implement-int32-rotation.patch

Review of attachment 8748958 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!
Attachment #8748958 - Flags: review?(bbouvier) → review+
https://hg.mozilla.org/mozilla-central/rev/51a6a9475189
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.