Bug 172306

Summary:

B3::Value::effects() says that having a fence range implies the fence bit, but on x86_64 we lower loadAcq/storeRel to load/store so the store-before-load fence bit orderings won't be honored

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
possible patch	none
the patch	none
the patch	msaboff: review+

Filip Pizlo

Reported 2017-05-18 12:51:41 PDT

As the title says.

Attachments
possible patch (22.72 KB, patch) 2017-05-18 15:51 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (32.61 KB, patch) 2017-05-18 16:48 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (32.60 KB, patch) 2017-05-18 17:29 PDT, Filip Pizlo	msaboff: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2017-05-18 12:52:44 PDT

I think that this means that MacroAssembler::storeRel on x86 should emit a xchg.

Filip Pizlo

Comment 2 2017-05-18 12:56:55 PDT

Seems like MacroAssembler even having methods like loadAcq and storeRel is super confusing. B3::LowerToAir should be making a deliberate choice about what acq/rel mean for x86, and I think that the only right interpretation is that rel means xchg.

Radar WebKit Bug Importer

Comment 3 2017-05-18 13:21:27 PDT

<rdar://problem/32279715>

Filip Pizlo

Comment 4 2017-05-18 15:51:10 PDT

Created attachment 310565 [details] possible patch

Filip Pizlo

Comment 5 2017-05-18 16:48:45 PDT

Created attachment 310575 [details] the patch

Filip Pizlo

Comment 6 2017-05-18 17:29:35 PDT

Created attachment 310578 [details] the patch