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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Sean Stangl [:sstangl]]

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.

Comment 1

[Sean Stangl [:sstangl]]

Attachment: 0001-Clean-up-ma_b-void-target.patch

Comment 2

[Sean Stangl [:sstangl]]

Attachment: 0002-Clean-up-MacroAssemblerARM-spacing.patch

Comment 3

[Sean Stangl [:sstangl]]

Attachment: 0003-Clean-up-ARM-Imm8-EncodeImm.patch

Comment 4

[Hannes Verschore [:h4writer]]

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 ...

Comment 5

[Hannes Verschore [:h4writer]]

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?

Comment 6

[Hannes Verschore [:h4writer]]

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?

Comment 8

[Sean Stangl [:sstangl]]

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.

Comment 9

[Hannes Verschore [:h4writer]]

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?

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd8e682dd7de
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c189431cdf7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ad95a1ec35

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd8e682dd7de
https://hg.mozilla.org/mozilla-central/rev/2c189431cdf7
https://hg.mozilla.org/mozilla-central/rev/c0ad95a1ec35