Ignore:
Timestamp:
May 19, 2017, 8:48:40 AM (8 years ago)
Author:
[email protected]
Message:

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
​https://bugs.webkit.org/show_bug.cgi?id=172306

Reviewed by Michael Saboff.

This changes B3 to emit xchg and its variants for fenced stores on x86. This ensures that
fenced stores cannot be reordered around other fenced instructions. Previously, B3 emitted
normal store instructions for fenced stores. That's wrong because then you get reorderings
that are possible in TSO but impossible in SC. Fenced instructions are supposed to be SC
with respect for each other.

This is imprecise. If you really just wanted a store-release, then every X86 store does this.
But, in B3, fenced stores are ARM-style store-release, meaning that they are fenced with
respect to all other fences. If we ever did want to say that something is a store release in
the traditional sense, then we'd want MemoryValue to have a fence flag. Then, having a fence
range without the fence flag would mean the traditional store-release, which lowers to a
normal store on x86. But to my knowledge, that traditional store-release is only useful for
unlocking spinlocks. We don't use spinlocks in JSC. Adaptive locks require CAS for unlock,
and B3 CAS is plenty fast. I think it's OK to have this small imprecision of giving clients
an ARM-style store-release on x86 using xchg.

The implication of this change is that the FTL no longer violates the SAB memory model.

  • assembler/MacroAssemblerX86Common.h:

(JSC::MacroAssemblerX86Common::xchg8):
(JSC::MacroAssemblerX86Common::xchg16):
(JSC::MacroAssemblerX86Common::xchg32):
(JSC::MacroAssemblerX86Common::loadAcq8): Deleted.
(JSC::MacroAssemblerX86Common::loadAcq8SignedExtendTo32): Deleted.
(JSC::MacroAssemblerX86Common::loadAcq16): Deleted.
(JSC::MacroAssemblerX86Common::loadAcq16SignedExtendTo32): Deleted.
(JSC::MacroAssemblerX86Common::loadAcq32): Deleted.
(JSC::MacroAssemblerX86Common::storeRel8): Deleted.
(JSC::MacroAssemblerX86Common::storeRel16): Deleted.
(JSC::MacroAssemblerX86Common::storeRel32): Deleted.

  • assembler/MacroAssemblerX86_64.h:

(JSC::MacroAssemblerX86_64::xchg64):
(JSC::MacroAssemblerX86_64::loadAcq64): Deleted.
(JSC::MacroAssemblerX86_64::storeRel64): Deleted.

  • b3/B3LowerToAir.cpp:

(JSC::B3::Air::LowerToAir::ArgPromise::inst):
(JSC::B3::Air::LowerToAir::trappingInst):
(JSC::B3::Air::LowerToAir::tryAppendStoreBinOp):
(JSC::B3::Air::LowerToAir::createStore):
(JSC::B3::Air::LowerToAir::storeOpcode):
(JSC::B3::Air::LowerToAir::appendStore):
(JSC::B3::Air::LowerToAir::append):
(JSC::B3::Air::LowerToAir::appendTrapping):
(JSC::B3::Air::LowerToAir::fillStackmap):
(JSC::B3::Air::LowerToAir::lower):

  • b3/air/AirKind.cpp:

(JSC::B3::Air::Kind::dump):

  • b3/air/AirKind.h:

(JSC::B3::Air::Kind::Kind):
(JSC::B3::Air::Kind::operator==):
(JSC::B3::Air::Kind::hash):

  • b3/air/AirLowerAfterRegAlloc.cpp:

(JSC::B3::Air::lowerAfterRegAlloc):

  • b3/air/AirLowerMacros.cpp:

(JSC::B3::Air::lowerMacros):

  • b3/air/AirOpcode.opcodes:
  • b3/air/AirValidate.cpp:
  • b3/air/opcode_generator.rb:
  • b3/testb3.cpp:

(JSC::B3::correctSqrt):
(JSC::B3::testSqrtArg):
(JSC::B3::testSqrtImm):
(JSC::B3::testSqrtMem):
(JSC::B3::testSqrtArgWithUselessDoubleConversion):
(JSC::B3::testSqrtArgWithEffectfulDoubleConversion):
(JSC::B3::testStoreRelAddLoadAcq32):
(JSC::B3::testTrappingLoad):
(JSC::B3::testTrappingStore):
(JSC::B3::testTrappingLoadAddStore):
(JSC::B3::testTrappingLoadDCE):

File:
1 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/b3/testb3.cpp

    r216989 r217127  
    44464446}
    44474447
     4448double correctSqrt(double value)
     4449{
     4450#if CPU(X86) || CPU(X86_64)
     4451    double result;
     4452    asm ("sqrtsd %1, %0" : "=x"(result) : "x"(value));
     4453    return result;
     4454#else   
     4455    return sqrt(value);
     4456#endif
     4457}
     4458
    44484459void testSqrtArg(double a)
    44494460{
    … …  
    44564467                root->appendNew<ArgumentRegValue>(proc, Origin(), FPRInfo::argumentFPR0)));
    44574468
    4458     CHECK(isIdentical(compileAndRun<double>(proc, a), sqrt(a)));
     4469    CHECK(isIdentical(compileAndRun<double>(proc, a), correctSqrt(a)));
    44594470}
    44604471
    … …  
    44684479        root->appendNew<Value>(proc, Sqrt, Origin(), argument));
    44694480
    4470     CHECK(isIdentical(compileAndRun<double>(proc), sqrt(a)));
     4481    CHECK(isIdentical(compileAndRun<double>(proc), correctSqrt(a)));
    44714482}
    44724483
    … …  
    44814492        root->appendNew<Value>(proc, Sqrt, Origin(), loadDouble));
    44824493
    4483     CHECK(isIdentical(compileAndRun<double>(proc, &a), sqrt(a)));
     4494    CHECK(isIdentical(compileAndRun<double>(proc, &a), correctSqrt(a)));
    44844495}
    44854496
    … …  
    44954506    root->appendNewControlValue(proc, Return, Origin(), result32);
    44964507
    4497     CHECK(isIdentical(compileAndRun<int32_t>(proc, bitwise_cast<int32_t>(a)), bitwise_cast<int32_t>(static_cast<float>(sqrt(a)))));
     4508    CHECK(isIdentical(compileAndRun<int32_t>(proc, bitwise_cast<int32_t>(a)), bitwise_cast<int32_t>(static_cast<float>(correctSqrt(a)))));
    44984509}
    44994510
    … …  
    45074518    root->appendNewControlValue(proc, Return, Origin(), result32);
    45084519
    4509     CHECK(isIdentical(compileAndRun<int32_t>(proc, bitwise_cast<int32_t>(a)), bitwise_cast<int32_t>(static_cast<float>(sqrt(a)))));
     4520    CHECK(isIdentical(compileAndRun<int32_t>(proc, bitwise_cast<int32_t>(a)), bitwise_cast<int32_t>(static_cast<float>(correctSqrt(a)))));
    45104521}
    45114522
    … …  
    45204531    root->appendNewControlValue(proc, Return, Origin(), result32);
    45214532
    4522     CHECK(isIdentical(compileAndRun<int32_t>(proc, &a), bitwise_cast<int32_t>(static_cast<float>(sqrt(a)))));
     4533    CHECK(isIdentical(compileAndRun<int32_t>(proc, &a), bitwise_cast<int32_t>(static_cast<float>(correctSqrt(a)))));
    45234534}
    45244535
    … …  
    45364547    root->appendNewControlValue(proc, Return, Origin(), result32);
    45374548
    4538     CHECK(isIdentical(compileAndRun<int32_t>(proc, bitwise_cast<int32_t>(a)), bitwise_cast<int32_t>(static_cast<float>(sqrt(a)))));
     4549    CHECK(isIdentical(compileAndRun<int32_t>(proc, bitwise_cast<int32_t>(a)), bitwise_cast<int32_t>(static_cast<float>(correctSqrt(a)))));
    45394550}
    45404551
    … …  
    45564567    double effect = 0;
    45574568    int32_t resultValue = compileAndRun<int32_t>(proc, bitwise_cast<int32_t>(a), &effect);
    4558     CHECK(isIdentical(resultValue, bitwise_cast<int32_t>(static_cast<float>(sqrt(a)))));
    4559     CHECK(isIdentical(effect, static_cast<double>(sqrt(a))));
     4569    CHECK(isIdentical(resultValue, bitwise_cast<int32_t>(static_cast<float>(correctSqrt(a)))));
     4570    double expected = static_cast<double>(correctSqrt(a));
     4571    CHECK(isIdentical(effect, expected));
    45604572}
    45614573
    … …  
    57945806        checkUsesInstruction(*code, "stl");
    57955807    }
     5808    if (isX86())
     5809        checkUsesInstruction(*code, "xchg");
    57965810    CHECK(!invoke<int>(*code, amount));
    57975811    CHECK(slot == 37 + amount);
    … …  
    58675881        checkUsesInstruction(*code, "stl");
    58685882    }
     5883    if (isX86())
     5884        checkUsesInstruction(*code, "xchg");
    58695885    CHECK(!invoke<int>(*code, amount));
    58705886    CHECK(slot == 37 + amount);
    … …  
    59065922        checkUsesInstruction(*code, "stl");
    59075923    }
     5924    if (isX86())
     5925        checkUsesInstruction(*code, "xchg");
    59085926    CHECK(!invoke<int>(*code, amount));
    59095927    CHECK(slot == 37 + amount);
    … …  
    59795997        checkUsesInstruction(*code, "stl");
    59805998    }
     5999    if (isX86())
     6000        checkUsesInstruction(*code, "xchg");
    59816001    CHECK(!invoke<int>(*code, amount));
    59826002    CHECK(slot == 37 + amount);
    … …  
    60486068        checkUsesInstruction(*code, "stl");
    60496069    }
     6070    if (isX86())
     6071        checkUsesInstruction(*code, "xchg");
    60506072    CHECK(!invoke<int>(*code, amount));
    60516073    CHECK(slot == 37000000000ll + amount);
    … …  
    1398014002    for (Air::BasicBlock* block : proc.code()) {
    1398114003        for (Air::Inst& inst : *block) {
    13982             if (inst.kind.traps)
     14004            if (inst.kind.effects)
    1398314005                trapsCount++;
    1398414006        }
    … …  
    1401314035    for (Air::BasicBlock* block : proc.code()) {
    1401414036        for (Air::Inst& inst : *block) {
    14015             if (inst.kind.traps)
     14037            if (inst.kind.effects)
    1401614038                trapsCount++;
    1401714039        }
    … …  
    1403914061    for (Air::BasicBlock* block : proc.code()) {
    1404014062        for (Air::Inst& inst : *block) {
    14041             if (inst.kind.traps)
     14063            if (inst.kind.effects)
    1404214064                traps = true;
    1404314065        }
    … …  
    1405914081    for (Air::BasicBlock* block : proc.code()) {
    1406014082        for (Air::Inst& inst : *block) {
    14061             if (inst.kind.traps)
     14083            if (inst.kind.effects)
    1406214084                trapsCount++;
    1406314085        }
Note: See TracChangeset for help on using the changeset viewer.