Bug 108261

Summary:

offlineasm BaseIndex handling is broken on ARM due to MIPS changes

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Balazs Kelemen <kbalazs>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, ddkilzer, fu, gergely, ggaren, kbalazs, kilvadyb, mark.lam, mhahnenberg, msaboff, oliver, psolanki, sam, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
the patch	none
proposed patch.	none

Description Filip Pizlo 2013-01-29 17:06:16 PST

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.

Comment 1 Filip Pizlo 2013-01-29 17:07:19 PST

Created attachment 185351 [details]
the patch

Comment 2 Filip Pizlo 2013-01-29 17:20:19 PST

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

Comment 3 Filip Pizlo 2013-01-29 17:20:38 PST

Comment on attachment 185351 [details]
the patch

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

Comment 4 Balazs Kilvady 2013-01-31 03:59:40 PST

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

Comment 5 Balazs Kilvady 2013-01-31 12:16:26 PST

Created attachment 185835 [details]
proposed patch.

Comment 6 Filip Pizlo 2013-01-31 12:28:52 PST

Comment on attachment 185835 [details]
proposed patch.

I like this.  r=me.

Comment 7 Mark Lam 2013-02-01 13:51:23 PST

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.

Comment 8 WebKit Review Bot 2013-02-01 14:01:34 PST

Comment on attachment 185835 [details]
proposed patch.

Clearing flags on attachment: 185835

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

Comment 9 WebKit Review Bot 2013-02-01 14:01:38 PST

All reviewed patches have been landed.  Closing bug.