[CodeGen] Introduce a generic MEMBARRIER instruction [mostly-nfc]

[reames]

Summary

This is a follow up to D141317 which extends the common code to include a target independent pseudo instruction. This is an alternative to (subset of) D92842 which tries to be as close to NFC as possible.

A couple things to call out for reviewers.

1. I couldn't find a way to do tablegen matching for a generic SDAG node to a generic MI. I hacked this up, but surely there's something obvious I'm missing here? If not, I can cleanup the hacky code; I just didn't bother in the hopes there was a table gen based approach I'd missed.

2. The test change in X86 is because we loose the scheduling information on the instruction. However, I think this was actually a bug in x86 since no instruction was emitted for a MEMBARRIER. Concluding that a meta instruction has latency just seems wrong?

3. More generally, is there anything specifically I need to do around scheduling info for the generic instruction? I'm unfamiliar with that part of our system.

4. I intentionally left some parts of D92842 out. Specifically, several of the changes in the X86 code (data independence and outlining) appear functional, and likely worthy of their own review. Additionally, I'm not handling ARM/AArch64 at all. Those targets need the ordering whereas none of the others do. I want to get this in and tested before retrofitting in ordering to support those targets.

[reames]

Rebase, and add RISCV since the change to add MEMBARRIER landed.

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

2908

I don't know of any target independent isel patterns today. I think all of the target independent opcodes are handled in this switch. The ones that do setNodeId(-1) are converted to MachineInstr in InstrEmitter.cpp.

Not sure what the "generalize op types" part of this comment means.

[reames]

Cleanup the manual selection since there doesn't appear to be a td means for this.

[lenary]

Broadly, this looks right. I'm glad stuff is finally moving on changes in this direction.

Why does this barrier not need an ordering, but ARM/AArch64 do? Some of the targets in this patch still have weak memory orderings (RISC-V). Is the plan to update the MEMBARRIER with the ordering later, in a way that won't actually affect the selection for these targets so far?

[reames]

In D141408#4041719, @lenary wrote:

Broadly, this looks right. I'm glad stuff is finally moving on changes in this direction.

Why does this barrier not need an ordering, but ARM/AArch64 do? Some of the targets in this patch still have weak memory orderings (RISC-V). Is the plan to update the MEMBARRIER with the ordering later, in a way that won't actually affect the selection for these targets so far?

I am working incrementally so that each patch is self contained, with as few test deltas as I can manage. As I said in the commit comment, this is merely staging, not a inherent disagreement on the result.

I've taken a quick look at ARM/AArch64 and don't see optimizations actually using the order on their compiler_barrier. We do have one backend which does, but that's AMDGPU which doesn't distinguish between a membarrier and a atomic_fence until late expansion.

[reames]

In D141408#4043653, @reames wrote:

I've taken a quick look at ARM/AArch64 and don't see optimizations actually using the order on their compiler_barrier.

The ARM/AArch64 patch is here: https://reviews.llvm.org/D141513. As expected, that didn't need the ordering.

llvm/include/llvm/CodeGen/SelectionDAGISel.h

private:
   void Select_INLINEASM(SDNode *N);
   void Select_READ_REGISTER(SDNode *Op);
   void Select_WRITE_REGISTER(SDNode *Op);
   void Select_UNDEF(SDNode *N);
   void CannotYetSelect(SDNode *N);
 
   void Select_FREEZE(SDNode *N);
   void Select_ARITH_FENCE(SDNode *N);
+  void Select_MEMBARRIER(SDNode *N);
 
   void pushStackMapLiveVariable(SmallVectorImpl<SDValue> &Ops, SDValue Operand,
                                 SDLoc DL);
   void Select_STACKMAP(SDNode *N);
   void Select_PATCHPOINT(SDNode *N);
 
 private:
   void DoInstructionSelection();

llvm/include/llvm/Support/TargetOpcodes.def

 HANDLE_TARGET_OPCODE(PATCHABLE_EVENT_CALL)
 
 /// Wraps a typed logging call and its argument with nop sleds. At runtime, this
 /// can be patched to insert instrumentation instructions.
 HANDLE_TARGET_OPCODE(PATCHABLE_TYPED_EVENT_CALL)
 
 HANDLE_TARGET_OPCODE(ICALL_BRANCH_FUNNEL)
 
+// This is a fence with the singlethread scope. It represents a compiler memory
+// barrier, but does not correspond to any generated instruction.
+HANDLE_TARGET_OPCODE(MEMBARRIER)
+
 /// The following generic opcodes are not supposed to appear after ISel.
 /// This is something we might want to relax, but for now, this is convenient
 /// to produce diagnostics.
 
 /// Instructions which should not exist past instruction selection, but do not
 /// generate code. These instructions only act as optimization hints.
 HANDLE_TARGET_OPCODE(G_ASSERT_SEXT)
 HANDLE_TARGET_OPCODE(G_ASSERT_ZEXT)

llvm/include/llvm/Target/Target.td

def FENTRY_CALL : StandardPseudoInstruction {
   let hasSideEffects = true;
 }
 def ICALL_BRANCH_FUNNEL : StandardPseudoInstruction {
   let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let AsmString = "";
   let hasSideEffects = true;
 }
+def MEMBARRIER : StandardPseudoInstruction {
+  let OutOperandList = (outs);
+  let InOperandList = (ins);
+  let AsmString = "";
+  let hasSideEffects = true;
+  let Size = 0;
+  let isMeta = true;
+}
 
 // Generic opcodes used in GlobalISel.
 include "llvm/Target/GenericOpcodes.td"
 
 //===----------------------------------------------------------------------===//
 // AsmParser - This class can be implemented by targets that wish to implement
 // .s file parsing.
 //

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

for (auto &MI : MBB) {
         break;
       case TargetOpcode::PSEUDO_PROBE:
         emitPseudoProbe(MI);
         break;
       case TargetOpcode::ARITH_FENCE:
         if (isVerbose())
           OutStreamer->emitRawComment("ARITH_FENCE");
         break;
+      case TargetOpcode::MEMBARRIER:
+        OutStreamer->emitRawComment("MEMBARRIER");
+        break;
       default:
         emitInstruction(&MI);
         if (CanDoExtraAnalysis) {
           MCInst MCI;
           MCI.setOpcode(MI.getOpcode());
           auto Name = OutStreamer->getMnemonic(MCI);
           auto I = MnemonicCounts.insert({Name, 0u});
           I.first->second++;

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

CurDAG->SelectNodeTo(N, TargetOpcode::COPY, N->getValueType(0),
                        N->getOperand(0));
 }
 
 void SelectionDAGISel::Select_ARITH_FENCE(SDNode *N) {
   CurDAG->SelectNodeTo(N, TargetOpcode::ARITH_FENCE, N->getValueType(0),
                        N->getOperand(0));
 }
 
+void SelectionDAGISel::Select_MEMBARRIER(SDNode *N) {
+  CurDAG->SelectNodeTo(N, TargetOpcode::MEMBARRIER, N->getValueType(0),
+                       N->getOperand(0));
+}
+
 void SelectionDAGISel::pushStackMapLiveVariable(SmallVectorImpl<SDValue> &Ops,
                                                 SDValue OpVal, SDLoc DL) {
   SDNode *OpNode = OpVal.getNode();
 
   // FrameIndex nodes should have been directly emitted to TargetFrameIndex
   // nodes at DAG-construction time.
   assert(OpNode->getOpcode() != ISD::FrameIndex);
 
case ISD::UNDEF:
     Select_UNDEF(NodeToMatch);
     return;
   case ISD::FREEZE:
     Select_FREEZE(NodeToMatch);
     return;
   case ISD::ARITH_FENCE:
     Select_ARITH_FENCE(NodeToMatch);
     return;
+  case ISD::MEMBARRIER:
+    Select_MEMBARRIER(NodeToMatch);
+    return;
   case ISD::STACKMAP:
     Select_STACKMAP(NodeToMatch);
     return;
   case ISD::PATCHPOINT:
     Select_PATCHPOINT(NodeToMatch);
     return;
   }
 

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

void RISCVAsmPrinter::emitInstruction(const MachineInstr *MI) {
   if (emitPseudoExpansionLowering(*OutStreamer, MI))
     return;
 
 
   switch (MI->getOpcode()) {
   case RISCV::HWASAN_CHECK_MEMACCESS_SHORTGRANULES:
     LowerHWASAN_CHECK_MEMACCESS(*MI);
     return;
-  case RISCV::PseudoMemBarrier:
-    OutStreamer->emitRawComment("MEMBARRIER");
-    return;
   }
 
   MCInst TmpInst;
   if (!lowerRISCVMachineInstrToMCInst(MI, TmpInst, *this))
     EmitToStreamer(*OutStreamer, TmpInst);
 }
 
 bool RISCVAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,

llvm/lib/Target/RISCV/RISCVInstrInfo.td

 
 let Predicates = [IsRV64] in {
 // Select W instructions if only the lower 32-bits of the result are used.
 def : Pat<(binop_allwusers<add> GPR:$rs1, (AddiPair:$rs2)),
           (ADDIW (ADDIW GPR:$rs1, (AddiPairImmLarge AddiPair:$rs2)),
                  (AddiPairImmSmall AddiPair:$rs2))>;
 }
 
-let hasSideEffects = 1, isMeta = 1 in
-def PseudoMemBarrier : Pseudo<(outs), (ins), [(membarrier)]>;
-
 //===----------------------------------------------------------------------===//
 // Standard extensions
 //===----------------------------------------------------------------------===//
 
 include "RISCVInstrInfoM.td"
 include "RISCVInstrInfoA.td"
 include "RISCVInstrInfoF.td"
 include "RISCVInstrInfoD.td"

llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp

case SystemZ::Serialize:
     if (MF->getSubtarget<SystemZSubtarget>().hasFastSerialization())
       LoweredMI = MCInstBuilder(SystemZ::BCRAsm)
         .addImm(14).addReg(SystemZ::R0D);
     else
       LoweredMI = MCInstBuilder(SystemZ::BCRAsm)
         .addImm(15).addReg(SystemZ::R0D);
     break;
 
-  // Emit nothing here but a comment if we can.
-  case SystemZ::MemBarrier:
-    OutStreamer->emitRawComment("MEMBARRIER");
-    return;
-
   // We want to emit "j .+2" for traps, jumping to the relative immediate field
   // of the jump instruction, which is an illegal instruction. We cannot emit a
   // "." symbol, so create and emit a temp label before the instruction and use
   // that instead.
   case SystemZ::Trap: {
     MCSymbol *DotSym = OutContext.createTempSymbol();
     OutStreamer->emitLabel(DotSym);
 

llvm/lib/Target/SystemZ/SystemZInstrInfo.td

 // Atomic operations
 //===----------------------------------------------------------------------===//
 
 // A serialization instruction that acts as a barrier for all memory
 // accesses, which expands to "bcr 14, 0".
 let hasSideEffects = 1 in
 def Serialize : Alias<2, (outs), (ins), []>;
 
-// A pseudo instruction that serves as a compiler barrier.
-let hasSideEffects = 1, hasNoSchedulingInfo = 1 in
-def MemBarrier : Pseudo<(outs), (ins), [(membarrier)]>;
-
 let Predicates = [FeatureInterlockedAccess1], Defs = [CC] in {
   def LAA   : LoadAndOpRSY<"laa",   0xEBF8, atomic_load_add_32, GR32>;
   def LAAG  : LoadAndOpRSY<"laag",  0xEBE8, atomic_load_add_64, GR64>;
   def LAAL  : LoadAndOpRSY<"laal",  0xEBFA, null_frag, GR32>;
   def LAALG : LoadAndOpRSY<"laalg", 0xEBEA, null_frag, GR64>;
   def LAN   : LoadAndOpRSY<"lan",   0xEBF4, atomic_load_and_32, GR32>;
   def LANG  : LoadAndOpRSY<"lang",  0xEBE4, atomic_load_and_64, GR64>;
   def LAO   : LoadAndOpRSY<"lao",   0xEBF6, atomic_load_or_32, GR32>;

llvm/lib/Target/SystemZ/SystemZLongBranch.cpp

 }
 
 static unsigned getInstSizeInBytes(const MachineInstr &MI,
                                    const SystemZInstrInfo *TII) {
   unsigned Size = TII->getInstSizeInBytes(MI);
   assert((Size ||
           // These do not have a size:
           MI.isDebugOrPseudoInstr() || MI.isPosition() || MI.isKill() ||
-          MI.isImplicitDef() || MI.getOpcode() == SystemZ::MemBarrier ||
+          MI.isImplicitDef() || MI.getOpcode() == TargetOpcode::MEMBARRIER ||
           // These have a size that may be zero:
           MI.isInlineAsm() || MI.getOpcode() == SystemZ::STACKMAP ||
           MI.getOpcode() == SystemZ::PATCHPOINT) &&
          "Missing size value for instruction.");
   return Size;
 }
 
 // Return a description of terminator instruction MI.

llvm/lib/Target/VE/VEInstrInfo.td

 // These calls are needed to probe the stack when allocating more over
 // %s8 (%sl - stack limit).
 
 let Uses = [SX11], hasSideEffects = 1 in
 def GETSTACKTOP : Pseudo<(outs I64:$dst), (ins),
                          "# GET STACK TOP",
                          [(set iPTR:$dst, (GetStackTop))]>;
 
-// MEMBARRIER
-let hasSideEffects = 1 in
-def MEMBARRIER : Pseudo<(outs), (ins), "# MEMBARRIER", [(membarrier)] >;
-
 //===----------------------------------------------------------------------===//
 // Other patterns
 //===----------------------------------------------------------------------===//
 
 // SETCC pattern matches
 //
 //   CMP  %tmp, lhs, rhs     ; compare lhs and rhs
 //   or   %res, 0, (0)1      ; initialize by 0

llvm/lib/Target/X86/X86InstrCompiler.td

 // Memory barriers
 
 let isCodeGenOnly = 1, Defs = [EFLAGS] in
 def OR32mi8Locked  : Ii8<0x83, MRM1m, (outs), (ins i32mem:$dst, i32i8imm:$zero),
                          "or{l}\t{$zero, $dst|$dst, $zero}", []>,
                          Requires<[Not64BitMode]>, OpSize32, LOCK,
                          Sched<[WriteALURMW]>;
 
-let hasSideEffects = 1, isMeta = 1 in
-def Int_MemBarrier : I<0, Pseudo, (outs), (ins),
-                     "#MEMBARRIER",
-                     [(membarrier)]>, Sched<[WriteLoad]>;
-
 // RegOpc corresponds to the mr version of the instruction
 // ImmOpc corresponds to the mi version of the instruction
 // ImmOpc8 corresponds to the mi8 version of the instruction
 // ImmMod corresponds to the instruction format of the mi and mi8 versions
 multiclass LOCK_ArithBinOp<bits<8> RegOpc, bits<8> ImmOpc, bits<8> ImmOpc8,
                            Format ImmMod, SDNode Op, string mnemonic> {
 let Defs = [EFLAGS], mayLoad = 1, mayStore = 1, isCodeGenOnly = 1,
     SchedRW = [WriteALURMW] in {

llvm/lib/Target/X86/X86MCInstLower.cpp

void X86AsmPrinter::emitInstruction(const MachineInstr *MI) {
   // Add comments for values loaded from constant pool.
   if (OutStreamer->isVerboseAsm())
     addConstantComments(MI, *OutStreamer);
 
   switch (MI->getOpcode()) {
   case TargetOpcode::DBG_VALUE:
     llvm_unreachable("Should be handled target independently");
 
-  // Emit nothing here but a comment if we can.
-  case X86::Int_MemBarrier:
-    OutStreamer->emitRawComment("MEMBARRIER");
-    return;
-
   case X86::EH_RETURN:
   case X86::EH_RETURN64: {
     // Lower these as normal, but add some comments.
     Register Reg = MI->getOperand(0).getReg();
     OutStreamer->AddComment(StringRef("eh_return, addr: %") +
                             X86ATTInstPrinter::getRegisterName(Reg));
     break;
   }

llvm/lib/Target/XCore/XCoreInstrInfo.td

 let usesCustomInserter = 1 in {
   def SELECT_CC : PseudoInstXCore<(outs GRRegs:$dst),
                               (ins GRRegs:$cond, GRRegs:$T, GRRegs:$F),
                               "# SELECT_CC PSEUDO!",
                               [(set GRRegs:$dst,
                                  (select GRRegs:$cond, GRRegs:$T, GRRegs:$F))]>;
 }
 
-let hasSideEffects = 1, isMeta = 1 in
-def Int_MemBarrier : PseudoInstXCore<(outs), (ins), "#MEMBARRIER",
-                                     [(membarrier)]>;
-
 //===----------------------------------------------------------------------===//
 // Instructions
 //===----------------------------------------------------------------------===//
 
 // Three operand short
 defm ADD : F3R_2RUS<0b00010, 0b10010, "add", add>;
 defm SUB : F3R_2RUS<0b00011, 0b10011, "sub", sub>;
 let hasSideEffects = 0 in {

llvm/test/CodeGen/X86/atomic-idempotent.ll

 ; X86-ATOM:       # %bb.0:
 ; X86-ATOM-NEXT:    #MEMBARRIER
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
+; X86-ATOM-NEXT:    nop
+; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    retl
   atomicrmw or ptr %p, i32 0 monotonic
   ret void
 }
 
 
 define void @or32_nouse_acquire(ptr %p) {
 ; X64-LABEL: or32_nouse_acquire:

 ; X86-ATOM:       # %bb.0:
 ; X86-ATOM-NEXT:    #MEMBARRIER
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
+; X86-ATOM-NEXT:    nop
+; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    retl
   atomicrmw or ptr %p, i32 0 acquire
   ret void
 }
 
 define void @or32_nouse_release(ptr %p) {
 ; X64-LABEL: or32_nouse_release:
 ; X64:       # %bb.0:

 ; X86-ATOM:       # %bb.0:
 ; X86-ATOM-NEXT:    #MEMBARRIER
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
+; X86-ATOM-NEXT:    nop
+; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    retl
   atomicrmw or ptr %p, i32 0 release
   ret void
 }
 
 define void @or32_nouse_acq_rel(ptr %p) {
 ; X64-LABEL: or32_nouse_acq_rel:
 ; X64:       # %bb.0:

 ; X86-ATOM:       # %bb.0:
 ; X86-ATOM-NEXT:    #MEMBARRIER
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    nop
+; X86-ATOM-NEXT:    nop
+; X86-ATOM-NEXT:    nop
 ; X86-ATOM-NEXT:    retl
   atomicrmw or ptr %p, i32 0 acq_rel
   ret void
 }
 
 define void @or32_nouse_seq_cst(ptr %p) {
 ; X64-LABEL: or32_nouse_seq_cst:
 ; X64:       # %bb.0:

llvm/unittests/MIR/MachineMetadata.cpp

body:             |
   EH_LABEL 0
   GC_LABEL 0
   DBG_VALUE $rax, $noreg, !13, !DIExpression(), debug-location !14
   DBG_LABEL 0
   LIFETIME_START 0
   LIFETIME_END 0
   PSEUDO_PROBE 6699318081062747564, 1, 0, 0
   $xmm0 = ARITH_FENCE $xmm0
-  Int_MemBarrier
+  MEMBARRIER
 ...
 )MIR";
 
   MachineModuleInfo MMI(TM.get());
   M = parseMIR(*TM, MIRString, "test0", MMI);
   ASSERT_TRUE(M);
 
   auto *MF = MMI.getMachineFunction(*M->getFunction("test0"));