Closed Bug 1207843 Opened 4 years ago Closed 4 years ago

ARM: Clean up Imm8::EncodeImm() and friends

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
Unspecified
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: sstangl, Assigned: sstangl)

References

Details

Attachments

(3 files)

Bug in three parts:

Part 1: Remove a bunch of dead code from ma_b().
Part 2: Fix spacing between functions in the ARM MacroAssembler.
Part 3: Rewrite Imm8::EncodeImm() to favor readability instead of micro-optimization.
Attachment #8665176 - Flags: review?(hv1989)
Attachment #8665178 - Flags: review?(hv1989)
Assignee: nobody → sstangl
Attachment #8665177 - Flags: review?(hv1989) → review+
Comment on attachment 8665178 [details] [diff] [review]
0003-Clean-up-ARM-Imm8-EncodeImm.patch

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

Not totally convinced this was needed. While it probably won't be noticeable, it is definitely slower. Also I don't think we found errors in this code explaining the need for rewrite.
Though I agree it is easier to read and to know there are no bugs in it.
And hey, the patch is already made ...
Attachment #8665178 - Flags: review?(hv1989) → review+
Comment on attachment 8665176 [details] [diff] [review]
0001-Clean-up-ma_b-void-target.patch

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

Can you give me a bit more context around this patch?
Which patch(es) make this code obsolete for Assembler::B_MOVWT and Assembler::B_LDR_BX and why?
Flags: needinfo?(sstangl)
Comment on attachment 8665176 [details] [diff] [review]
0001-Clean-up-ma_b-void-target.patch

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

Can you give me a bit more context around this patch?
Which patch(es) make this code obsolete for Assembler::B_MOVWT and Assembler::B_LDR_BX and why?
Attachment #8665176 - Flags: review?(hv1989)
Duplicate of this bug: 1207841
Those paths have been dead code for a very long time, since b_type(), used as the argument in the switch, is only ever B_LDR.

The last bug to touch this code was bug 725584. Even before that patch, the vast majority of ma_b() was dead code. It has been dead code since before February 2012.

Hopefully that is sufficient context. If you need more, it will be difficult to look up.
Flags: needinfo?(sstangl)
Attachment #8665176 - Flags: review?(hv1989)
Comment on attachment 8665176 [details] [diff] [review]
0001-Clean-up-ma_b-void-target.patch

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

Sorry for the delay. Looks good to me. +1 for dead code removal.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +1469,3 @@
>  void
>  MacroAssemblerARM::ma_b(void* target, Relocation::Kind reloc, Assembler::Condition c)
>  {

Can we remove the "Relocation::Kind reloc" argument which serves no purpose currently?
Attachment #8665176 - Flags: review?(hv1989) → review+
Depends on: 1220275
You need to log in before you can comment on or make changes to this bug.