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/B3LowerToAir.cpp

    r215642 r217127  
    273273        {
    274274            Inst result(std::forward<Args>(args)...);
    275             result.kind.traps |= m_traps;
     275            result.kind.effects |= m_traps;
    276276            m_wasWrapped = true;
    277277            return result;
    … …  
    569569    {
    570570        Inst result(std::forward<Args>(args)...);
    571         result.kind.traps |= traps;
     571        result.kind.effects |= traps;
    572572        return result;
    573573    }
    … …  
    962962            return false;
    963963       
    964         // On x86, all stores have fences, and this isn't reordering the store itself.
    965         if (!isX86() && m_value->as<MemoryValue>()->hasFence())
     964        if (m_value->as<MemoryValue>()->hasFence())
    966965            return false;
    967966       
    … …  
    10181017    }
    10191018
    1020     Inst createStore(Air::Opcode move, Value* value, const Arg& dest)
    1021     {
    1022         if (imm(value) && isValidForm(move, Arg::Imm, dest.kind()))
     1019    Inst createStore(Air::Kind move, Value* value, const Arg& dest)
     1020    {
     1021        if (imm(value) && isValidForm(move.opcode, Arg::Imm, dest.kind()))
    10231022            return Inst(move, m_value, imm(value), dest);
    10241023
    … …  
    10261025    }
    10271026   
    1028     Air::Opcode storeOpcode(Width width, Bank bank, bool release)
     1027    Air::Opcode storeOpcode(Width width, Bank bank)
    10291028    {
    10301029        switch (width) {
    10311030        case Width8:
    10321031            RELEASE_ASSERT(bank == GP);
    1033             return release ? StoreRel8 : Air::Store8;
     1032            return Air::Store8;
    10341033        case Width16:
    10351034            RELEASE_ASSERT(bank == GP);
    1036             return release ? StoreRel16 : Air::Store16;
     1035            return Air::Store16;
    10371036        case Width32:
    10381037            switch (bank) {
    10391038            case GP:
    1040                 return release ? StoreRel32 : Move32;
     1039                return Move32;
    10411040            case FP:
    1042                 RELEASE_ASSERT(!release);
    10431041                return MoveFloat;
    10441042            }
    … …  
    10481046            switch (bank) {
    10491047            case GP:
    1050                 return release ? StoreRel64 : Move;
     1048                return Move;
    10511049            case FP:
    1052                 RELEASE_ASSERT(!release);
    10531050                return MoveDouble;
    10541051            }
    … …  
    10581055    }
    10591056   
    1060     Air::Opcode storeOpcode(Value* value)
     1057    void appendStore(Value* value, const Arg& dest)
    10611058    {
    10621059        MemoryValue* memory = value->as<MemoryValue>();
    10631060        RELEASE_ASSERT(memory->isStore());
    1064         return storeOpcode(memory->accessWidth(), memory->accessBank(), memory->hasFence());
    1065     }
    1066 
    1067     Inst createStore(Value* value, const Arg& dest)
    1068     {
    1069         Air::Opcode moveOpcode = storeOpcode(value);
    1070         return createStore(moveOpcode, value->child(0), dest);
    1071     }
    1072 
    1073     template<typename... Args>
    1074     void appendStore(Args&&... args)
    1075     {
    1076         append(trappingInst(m_value, createStore(std::forward<Args>(args)...)));
    1077     }
    1078    
     1061
     1062        Air::Kind kind;
     1063        if (memory->hasFence()) {
     1064            RELEASE_ASSERT(memory->accessBank() == GP);
     1065           
     1066            if (isX86()) {
     1067                kind = OPCODE_FOR_WIDTH(Xchg, memory->accessWidth());
     1068                kind.effects = true;
     1069                Tmp swapTmp = m_code.newTmp(GP);
     1070                append(relaxedMoveForType(memory->accessType()), tmp(memory->child(0)), swapTmp);
     1071                append(kind, swapTmp, dest);
     1072                return;
     1073            }
     1074           
     1075            kind = OPCODE_FOR_WIDTH(StoreRel, memory->accessWidth());
     1076        } else
     1077            kind = storeOpcode(memory->accessWidth(), memory->accessBank());
     1078       
     1079        kind.effects |= memory->traps();
     1080       
     1081        append(createStore(kind, memory->child(0), dest));
     1082    }
     1083
    10791084    Air::Opcode moveForType(Type type)
    10801085    {
    … …  
    11521157
    11531158    template<typename... Arguments>
    1154     void append(Air::Opcode opcode, Arguments&&... arguments)
    1155     {
    1156         m_insts.last().append(Inst(opcode, m_value, std::forward<Arguments>(arguments)...));
     1159    void append(Air::Kind kind, Arguments&&... arguments)
     1160    {
     1161        m_insts.last().append(Inst(kind, m_value, std::forward<Arguments>(arguments)...));
    11571162    }
    11581163   
    11591164    template<typename... Arguments>
    1160     void appendTrapping(Air::Opcode opcode, Arguments&&... arguments)
    1161     {
    1162         m_insts.last().append(trappingInst(m_value, opcode, m_value, std::forward<Arguments>(arguments)...));
     1165    void appendTrapping(Air::Kind kind, Arguments&&... arguments)
     1166    {
     1167        m_insts.last().append(trappingInst(m_value, kind, m_value, std::forward<Arguments>(arguments)...));
    11631168    }
    11641169   
    … …  
    12551260            case ValueRep::StackArgument:
    12561261                arg = Arg::callArg(value.rep().offsetFromSP());
    1257                 appendStore(moveForType(value.value()->type()), value.value(), arg);
     1262                append(trappingInst(m_value, createStore(moveForType(value.value()->type()), value.value(), arg)));
    12581263                break;
    12591264            default:
    … …  
    24152420        case Load: {
    24162421            MemoryValue* memory = m_value->as<MemoryValue>();
    2417             Air::Opcode opcode = Air::Oops;
     2422            Air::Kind kind = moveForType(memory->type());
    24182423            if (memory->hasFence()) {
    2419                 switch (memory->type()) {
    2420                 case Int32:
    2421                     opcode = LoadAcq32;
    2422                     break;
    2423                 case Int64:
    2424                     opcode = LoadAcq64;
    2425                     break;
    2426                 default:
    2427                     RELEASE_ASSERT_NOT_REACHED();
    2428                     break;
    2429                 }
    2430             } else
    2431                 opcode = moveForType(memory->type());
    2432             append(trappingInst(m_value, opcode, m_value, addr(m_value), tmp(m_value)));
     2424                if (isX86())
     2425                    kind.effects = true;
     2426                else {
     2427                    switch (memory->type()) {
     2428                    case Int32:
     2429                        kind = LoadAcq32;
     2430                        break;
     2431                    case Int64:
     2432                        kind = LoadAcq64;
     2433                        break;
     2434                    default:
     2435                        RELEASE_ASSERT_NOT_REACHED();
     2436                        break;
     2437                    }
     2438                }
     2439            }
     2440            append(trappingInst(m_value, kind, m_value, addr(m_value), tmp(m_value)));
    24332441            return;
    24342442        }
    24352443           
    24362444        case Load8S: {
    2437             Air::Opcode opcode = m_value->as<MemoryValue>()->hasFence() ? LoadAcq8SignedExtendTo32 : Load8SignedExtendTo32;
    2438             append(trappingInst(m_value, opcode, m_value, addr(m_value), tmp(m_value)));
     2445            Air::Kind kind = Load8SignedExtendTo32;
     2446            if (m_value->as<MemoryValue>()->hasFence()) {
     2447                if (isX86())
     2448                    kind.effects = true;
     2449                else
     2450                    kind = LoadAcq8SignedExtendTo32;
     2451            }
     2452            append(trappingInst(m_value, kind, m_value, addr(m_value), tmp(m_value)));
    24392453            return;
    24402454        }
    24412455
    24422456        case Load8Z: {
    2443             Air::Opcode opcode = m_value->as<MemoryValue>()->hasFence() ? LoadAcq8 : Load8;
    2444             append(trappingInst(m_value, opcode, m_value, addr(m_value), tmp(m_value)));
     2457            Air::Kind kind = Load8;
     2458            if (m_value->as<MemoryValue>()->hasFence()) {
     2459                if (isX86())
     2460                    kind.effects = true;
     2461                else
     2462                    kind = LoadAcq8;
     2463            }
     2464            append(trappingInst(m_value, kind, m_value, addr(m_value), tmp(m_value)));
    24452465            return;
    24462466        }
    24472467
    24482468        case Load16S: {
    2449             Air::Opcode opcode = m_value->as<MemoryValue>()->hasFence() ? LoadAcq16SignedExtendTo32 : Load16SignedExtendTo32;
    2450             append(trappingInst(m_value, opcode, m_value, addr(m_value), tmp(m_value)));
     2469            Air::Kind kind = Load16SignedExtendTo32;
     2470            if (m_value->as<MemoryValue>()->hasFence()) {
     2471                if (isX86())
     2472                    kind.effects = true;
     2473                else
     2474                    kind = LoadAcq16SignedExtendTo32;
     2475            }
     2476            append(trappingInst(m_value, kind, m_value, addr(m_value), tmp(m_value)));
    24512477            return;
    24522478        }
    24532479
    24542480        case Load16Z: {
    2455             Air::Opcode opcode = m_value->as<MemoryValue>()->hasFence() ? LoadAcq16 : Load16;
    2456             append(trappingInst(m_value, opcode, m_value, addr(m_value), tmp(m_value)));
     2481            Air::Kind kind = Load16;
     2482            if (m_value->as<MemoryValue>()->hasFence()) {
     2483                if (isX86())
     2484                    kind.effects = true;
     2485                else
     2486                    kind = LoadAcq16;
     2487            }
     2488            append(trappingInst(m_value, kind, m_value, addr(m_value), tmp(m_value)));
    24572489            return;
    24582490        }
Note: See TracChangeset for help on using the changeset viewer.