[PATCH] Add AtomicExpandPass::bracketInstWithFences, and use it whenever getInsertFencesForAtomic would trigger in SelectionDAGBuilder

Robin Morisset morisset at google.com
Mon Sep 22 10:37:32 PDT 2014


Thanks for the review, I will fix the comments as described below.

================
Comment at: include/llvm/Target/TargetLowering.h:963
@@ -962,2 +962,3 @@
   /// RMW and CmpXchg set both IsStore and IsLoad to true.
   /// Backends with !getInsertFencesForAtomic() should keep a no-op here.
+  virtual Instruction* emitLeadingFence(IRBuilder<> &Builder, AtomicOrdering Ord,
----------------
jfb wrote:
> What should the return value be? Are there cases where a backend emits more than one instruction?
The return value is either nullptr (if the backend did not insert anything) or a pointer to the inserted (IR-level) instruction. There is no reason for a backend to insert several (IR-level) instructions, the only thing that makes sense for a backend to insert is an intrinsic corresponding to its hardware fence(s). Even if it requires several machine instructions for a fence, it can still be just one IR-level instruction.
I will add a comment to that effect.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:167
@@ -117,3 +166,3 @@
   // The only 64-bit load guaranteed to be single-copy atomic by ARM is
   // an ldrexd (A3.5.3).
   Value *Val =
----------------
jfb wrote:
> Should this still be here, since the code wants to handle more than just ARM?
Probably not exactly as worded. I will reword it by saying that on some architectures load-linked is atomic for bigger sizes, and that for example on ARM...

http://reviews.llvm.org/D5179






More information about the llvm-commits mailing list