Bug 132330 - LLINT loadisFromInstruction doesn't need special case for big endians
Summary: LLINT loadisFromInstruction doesn't need special case for big endians
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 128743 132333
  Show dependency treegraph
 
Reported: 2014-04-29 04:30 PDT by Tomas Popela
Modified: 2014-04-29 07:06 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (2.67 KB, patch)
2014-04-29 04:33 PDT, Tomas Popela
mark.lam: review-
Details | Formatted Diff | Diff
Proposed patch v2 (2.68 KB, patch)
2014-04-29 06:28 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Popela 2014-04-29 04:30:02 PDT
The change introduced in r167076 was wrong. We cannot apply the offset adjustment on every loadisFromInstruction usage as the instruction (UnlinkedInstruction) is declared as an union (i.e. with the int32_t operand variable). The offset of the union members will be the same as the offset of the first one, that is 0. The behavior here is the same on little and big endian architectures us we don't need special case for big endians.
Comment 1 Tomas Popela 2014-04-29 04:33:42 PDT
Created attachment 230365 [details]
Proposed patch
Comment 2 Mark Lam 2014-04-29 06:10:08 PDT
Comment on attachment 230365 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230365&action=review

Thanks for fixing this.  If you update the ChangeLog comment, we should be good to go.

> Source/JavaScriptCore/ChangeLog:9
> +        The change introduced in r167076 was wrong. We cannot apply the offset
> +        adjustment on every loadisFromInstruction usage as the instruction

Please change “cannot” to “should not” and remove “every”.  As it read previously, it implied that there are some cases where the offset should be applied, which is not true.  It is always wrong to applied that offset.

> Source/JavaScriptCore/ChangeLog:11
> +        operand variable). The offset of the union members will be the

Please change “union” to “other union”.

> Source/JavaScriptCore/ChangeLog:13
> +        same on little and big endian architectures thus we don't need

Missing period between “architectures. Thus”.
Comment 3 Tomas Popela 2014-04-29 06:28:43 PDT
Created attachment 230373 [details]
Proposed patch v2

Fixes after Mark's review
Comment 4 Mark Lam 2014-04-29 06:29:53 PDT
Comment on attachment 230373 [details]
Proposed patch v2

r=me
Comment 5 WebKit Commit Bot 2014-04-29 07:06:37 PDT
Comment on attachment 230373 [details]
Proposed patch v2

Clearing flags on attachment: 230373

Committed r167929: <http://trac.webkit.org/changeset/167929>
Comment 6 WebKit Commit Bot 2014-04-29 07:06:40 PDT
All reviewed patches have been landed.  Closing bug.