Bug 108261 – offlineasm BaseIndex handling is broken on ARM due to MIPS changes

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

The riscLowerMalformedAddresses code is intended to only lower BaseIndex if the target architecture can't handle that specific form of BaseIndex.  And when it does lower BaseIndex, it's supposed to lower it to a leap instruction, in recognition of the fact that the target architecture almost certainly has a more efficient way of handling add and shift in a single instruction.

Currently the mips.rb backend overrides riscLowerMalformedAddressesRecurse for BaseIndex, effectively deactivating that optimization in its entirety.

I will disable the overriding for now, which will bring ARM back to its proper glory.  I'll leave it to the MIPS port to sort this out.

Bottom line: you shouldn't be overriding methods in mips.rb, since mips.rb is included in *all* ports.  This is by design since offlineasm is meant to be able to work with fat binaries, where a single invocation simultaneously generates code for multiple architectures.

[Filip Pizlo]

Created attachment 185351 [details]
the patch

[Filip Pizlo]

MIPS code disabled in http://trac.webkit.org/changeset/141189

[Filip Pizlo]

Comment on attachment 185351 [details]
the patch

Clearing flags since this landed in http://trac.webkit.org/changeset/141189

[Balazs Kilvady]

(In reply to comment #0)
> The riscLowerMalformedAddresses code is intended to only lower BaseIndex if the target architecture can't handle that specific form of BaseIndex.  And when it does lower BaseIndex, it's supposed to lower it to a leap instruction, in recognition of the fact that the target architecture almost certainly has a more efficient way of handling add and shift in a single instruction.
> 
> Currently the mips.rb backend overrides riscLowerMalformedAddressesRecurse for BaseIndex, effectively deactivating that optimization in its entirety.
> 
> I will disable the overriding for now, which will bring ARM back to its proper glory.  I'll leave it to the MIPS port to sort this out.
> 
> Bottom line: you shouldn't be overriding methods in mips.rb, since mips.rb is included in *all* ports.  This is by design since offlineasm is meant to be able to work with fat binaries, where a single invocation simultaneously generates code for multiple architectures.

Thanks for the explanation, I am working on a proper solution.

[Balazs Kilvady]

Created attachment 185835 [details]
proposed patch.

[Filip Pizlo]

Comment on attachment 185835 [details]
proposed patch.

I like this.  r=me.

[Mark Lam]

Comment on attachment 185835 [details]
proposed patch.

Since Filip r+'d this already and the cq? was added after, I'll cq+ it to move it along.

[WebKit Review Bot]

Comment on attachment 185835 [details]
proposed patch.

Clearing flags on attachment: 185835

Committed r141641: <http://trac.webkit.org/changeset/141641>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.