[RISCV] Avoid emitting hardware fences for singlethread fences

[reames]

Summary

singlethread fences only synchronize with code running on the same hardware thread (i.e. signal handlers). Because of this, we need to prevent instruction reordering, but do not need to emit hardware fence instructions.

The implementation strategy here matches many other backends. The main motivation of this patch is to introduce the MEMBARRIER node and get some test coverage for it.

[jrtc27]

If every backend is doing it shouldn't this be a generic thing rather than copy/pasted to every backend?

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1851

This isn't standard naming for pseudos, but maybe other backends do it this way too?...

[reames]

In D141311#4037290, @jrtc27 wrote:

If every backend is doing it shouldn't this be a generic thing rather than copy/pasted to every backend?

Agreed. I looked briefly at doing this. I think we could probably share the SDAG pieces; the pseudos look less obvious. Happy to take a pass at this, but I'd prefer that be non-blocking for this change. You okay with that?

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1851

I honestly copied this from X86 without trying to understand it closely. If you have suggestions, happy to apply them.

[craig.topper]

There was an attempt to make a target independent compiler barrier here. https://reviews.llvm.org/D92842 I haven't re-read through the discussion yet.

[jrtc27]

In D141311#4037302, @reames wrote:

In D141311#4037290, @jrtc27 wrote:

If every backend is doing it shouldn't this be a generic thing rather than copy/pasted to every backend?

Agreed. I looked briefly at doing this. I think we could probably share the SDAG pieces; the pseudos look less obvious. Happy to take a pass at this, but I'd prefer that be non-blocking for this change. You okay with that?

Yeah, I just wish it were done that way from the start :( busywork as a result

llvm/lib/Target/RISCV/RISCVInstrInfo.td

1851

Normally it'd be riscv_membarrier defined at the top of the file and PseudoMEMBARRIER defined here (or in the instructions section)

[reames]

In D141311#4037335, @jrtc27 wrote:

In D141311#4037302, @reames wrote:

In D141311#4037290, @jrtc27 wrote:

If every backend is doing it shouldn't this be a generic thing rather than copy/pasted to every backend?

Agreed. I looked briefly at doing this. I think we could probably share the SDAG pieces; the pseudos look less obvious. Happy to take a pass at this, but I'd prefer that be non-blocking for this change. You okay with that?

Yeah, I just wish it were done that way from the start :( busywork as a result

The SDAG part turned out to be easier than I'd realized, and is now posted: https://reviews.llvm.org/D141317

I'm going to rebase this patch once that one lands.

[reames]

Rebase

[asb]

LGTM - two minor nits inline.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

3677

handles => handlers

3687

Accidental extra whitespace?

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

if (Subtarget.hasStdExtA()) {
     setMaxAtomicSizeInBitsSupported(Subtarget.getXLen());
     setMinCmpXchgSizeInBits(32);
   } else if (Subtarget.hasForcedAtomics()) {
     setMaxAtomicSizeInBitsSupported(Subtarget.getXLen());
   } else {
     setMaxAtomicSizeInBitsSupported(0);
   }
 
+  setOperationAction(ISD::ATOMIC_FENCE, MVT::Other, Custom);
+
   setBooleanContents(ZeroOrOneBooleanContent);
 
   if (Subtarget.hasVInstructions()) {
     setBooleanVectorContents(ZeroOrOneBooleanContent);
 
     setOperationAction(ISD::VSCALE, XLenVT, Custom);
 
     // RVV intrinsics may have illegal operands.
RISCVMatInt::InstSeq Seq =
       RISCVMatInt::generateInstSeq(Imm, Subtarget.getFeatureBits());
   if (Seq.size() <= Subtarget.getMaxBuildIntsCost())
     return Op;
 
   // Expand to a constant pool using the default expansion code.
   return SDValue();
 }
 
+static SDValue LowerATOMIC_FENCE(SDValue Op, SelectionDAG &DAG) {
+  SDLoc dl(Op);
+  SyncScope::ID FenceSSID =
+      static_cast<SyncScope::ID>(Op.getConstantOperandVal(2));
+
+  // singlethread fences only synchronize with signal handlers on the same
+  // thread and thus only need to preserve instruction order, not actually
+  // enforce memory ordering.
+  if (FenceSSID == SyncScope::SingleThread)
+    // MEMBARRIER is a compiler barrier; it codegens to a no-op.
+    return DAG.getNode(ISD::MEMBARRIER, dl, MVT::Other, Op.getOperand(0));
+
+  return Op;
+}
+
 SDValue RISCVTargetLowering::LowerOperation(SDValue Op,
                                             SelectionDAG &DAG) const {
   switch (Op.getOpcode()) {
   default:
     report_fatal_error("unimplemented operand");
+  case ISD::ATOMIC_FENCE:
+    return LowerATOMIC_FENCE(Op, DAG);
   case ISD::GlobalAddress:
     return lowerGlobalAddress(Op, DAG);
   case ISD::BlockAddress:
     return lowerBlockAddress(Op, DAG);
   case ISD::ConstantPool:
     return lowerConstantPool(Op, DAG);
   case ISD::JumpTable:
     return lowerJumpTable(Op, DAG);

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"
 include "RISCVInstrInfoC.td"
 include "RISCVInstrInfoZb.td"
 include "RISCVInstrInfoZk.td"

llvm/test/CodeGen/RISCV/atomic-fence.ll

 ; CHECK-NEXT:    ret
   fence seq_cst
   ret void
 }
 
 define void @fence_singlethread_acquire() nounwind {
 ; CHECK-LABEL: fence_singlethread_acquire:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fence r, rw
+; CHECK-NEXT:    #MEMBARRIER
 ; CHECK-NEXT:    ret
   fence syncscope("singlethread") acquire
   ret void
 }
 
 define void @fence_singlethread_release() nounwind {
 ; CHECK-LABEL: fence_singlethread_release:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fence rw, w
+; CHECK-NEXT:    #MEMBARRIER
 ; CHECK-NEXT:    ret
   fence syncscope("singlethread") release
   ret void
 }
 
 define void @fence_singlethread_acq_rel() nounwind {
 ; CHECK-LABEL: fence_singlethread_acq_rel:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fence.tso
+; CHECK-NEXT:    #MEMBARRIER
 ; CHECK-NEXT:    ret
   fence syncscope("singlethread") acq_rel
   ret void
 }
 
 define void @fence_singlethread_seq_cst() nounwind {
 ; CHECK-LABEL: fence_singlethread_seq_cst:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fence rw, rw
+; CHECK-NEXT:    #MEMBARRIER
 ; CHECK-NEXT:    ret
   fence syncscope("singlethread") seq_cst
   ret void
 }