LLINT loadisFromInstruction doesn't need special case for big endians

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tomas Popela]

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.

[Tomas Popela]

Created attachment 230365 [details]
Proposed patch

[Mark Lam]

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

[Tomas Popela]

Created attachment 230373 [details]
Proposed patch v2

Fixes after Mark's review

[Mark Lam]

Comment on attachment 230373 [details]
Proposed patch v2

r=me

[WebKit Commit Bot]

Comment on attachment 230373 [details]
Proposed patch v2

Clearing flags on attachment: 230373

Committed r167929: <http://trac.webkit.org/changeset/167929>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.