172383 – DFG::SpeculativeJIT::pickCanTrample() is wrongly ignoring result registers.

Bug 172383 - DFG::SpeculativeJIT::pickCanTrample() is wrongly ignoring result registers.

Summary: DFG::SpeculativeJIT::pickCanTrample() is wrongly ignoring result registers.

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Mark Lam

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2017-05-19 14:15 PDT by Mark Lam
Modified:	2017-05-19 18:00 PDT (History)
CC List:	8 users (show)

See Also:

Attachments
proposed patch. (30.01 KB, patch) 2017-05-19 14:26 PDT, Mark Lam	fpizlo: review+	Details \| Formatted Diff \| Diff
Patch for re-landing. (29.99 KB, patch) 2017-05-19 17:49 PDT, Mark Lam	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

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

Description Mark Lam 2017-05-19 14:15:38 PDT

pickCanTrample() is wrongly assuming that one of regT0 and regT1 is always available as a scratch register.  This assumption is wrong if this canTrample register is used for a silentFill() after an operation that returns a result in regT0 or regT1.

Turns out the only reason we need the canTrample register is for SetDoubleConstant.  We can remove the need for this canTrample register by introducing a moveDouble() pseudo instruction in the MacroAssembler to do the job using the scratchRegister() on X86_64 or the dataMemoryTempRegister() on ARM64.  In so doing, we can simplify the silentFill() code and eliminate the bug.

<rdar://problem/31418651>

Comment 1 Mark Lam 2017-05-19 14:26:41 PDT

Created attachment 310700 [details]
proposed patch.

Comment 2 Mark Lam 2017-05-19 15:25:54 PDT

Thanks for the review.  Landed in r217156: <http://trac.webkit.org/r217156>.

Comment 3 Ryan Haddad 2017-05-19 16:25:15 PDT

Reverted r217156 for reason:

This change broke the iOS build.

Committed r217169: <http://trac.webkit.org/changeset/217169>

Comment 4 Mark Lam 2017-05-19 17:49:11 PDT

Created attachment 310735 [details]
Patch for re-landing.

Comment 5 Mark Lam 2017-05-19 17:59:05 PDT

Comment on attachment 310735 [details]
Patch for re-landing.

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

> Source/JavaScriptCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +

Will remove this before landing.

Comment 6 Mark Lam 2017-05-19 18:00:17 PDT

Landed in r217179: <http://trac.webkit.org/r217179>.