[PATCH] D92842: [RFC][SelectionDAG] Add Target-Independent Compiler Barrier

[Sam Elliott via Phabricator via llvm-commits]

lenary added inline comments.


================
Comment at: llvm/include/llvm/Support/TargetOpcodes.def:212-215
+// This is a fence with the singlethread scope. It has a single operand, which
+// is the ordering requested. This instruction has `mayLoad` and `mayStore`, so
+// that memory operations are not moved around it.
+HANDLE_TARGET_OPCODE(COMPILER_BARRIER)
----------------
arsenm wrote:
> I think the scope should also be parameterized. This also does not match the description of the defined instruction which is missing the ordering operand
The documentation about `ordering` is out of date, I did include it before, but then ran into x86 issues, so removed it. I'm looking at bringing it back with @craig.topper's suggestions.

Because this barrier explicitly does not translate to machine code (that would be a place where you used a target-dependent fence or similar), I don't want to suggest this has any syncscope that is not singlethread, but maybe that's wrong?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:28637
+  // COMPILER_BARRIER codegens to a 0-byte instruction.
+  return SDValue(DAG.getMachineNode(TargetOpcode::COMPILER_BARRIER, dl, MVT::Other, Op.getOperand(0)), 0);
 }
----------------
craig.topper wrote:
> lenary wrote:
> > craig.topper wrote:
> > > Would it make sense to have a SelectionDAG::getCompilerBarrier method that would hide most of the boilerplate for creating these nodes?
> > Almost certainly.
> > 
> > One thing I did run into was I wanted to keep the ordering in the barrier around for later passes - but I ran into issues on x86 where the immediate I was using to represent it was getting allocated into a register and that caused instruction verification to fail. I could look at bringing that back if people thought it was useful (for instance, later Machine Passes may want to know if they can hoist/sink loads and stores past a barrier with more granularity than just "No").
> Can you use getTargetConstant? That should never end up in a register.
I'll try again, I cannot remember exactly what I used for that version of the code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92842/new/

https://reviews.llvm.org/D92842