Bug 132330

Summary:

LLINT loadisFromInstruction doesn't need special case for big endians

Product:

WebKit

Reporter:

Tomas Popela <tpopela>

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, mark.lam

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

128743, 132333

Attachments:

Description	Flags
Proposed patch	mark.lam: review-
Proposed patch v2	none

Tomas Popela

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

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Tomas Popela

Comment 1 2014-04-29 04:33:42 PDT

Created attachment 230365 [details] Proposed patch

Mark Lam

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

Tomas Popela

Comment 3 2014-04-29 06:28:43 PDT

Created attachment 230373 [details] Proposed patch v2 Fixes after Mark's review

Mark Lam

Comment 4 2014-04-29 06:29:53 PDT

Comment on attachment 230373 [details] Proposed patch v2 r=me

WebKit Commit Bot

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

WebKit Commit Bot

Comment 6 2014-04-29 07:06:40 PDT

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.