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

Filip Pizlo

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

Attachments
the patch (1.23 KB, patch) 2013-01-29 17:07 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
proposed patch. (3.34 KB, patch) 2013-01-31 12:16 PST, Balazs Kilvady	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

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

Created attachment 185351 [details] the patch

Filip Pizlo

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

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

Filip Pizlo

Comment 3 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

Balazs Kilvady

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

Balazs Kilvady

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

Created attachment 185835 [details] proposed patch.

Filip Pizlo

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

Comment on attachment 185835 [details] proposed patch. I like this. r=me.

Mark Lam

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

WebKit Review Bot

Comment 8 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>

WebKit Review Bot

Comment 9 2013-02-01 14:01:38 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.