[PATCH] Refactor AtomicExpandPass and add a generic isAtomic() method to Instruction

JF Bastien jfb at chromium.org
Mon Aug 25 13:17:26 PDT 2014


This lgtm with one comment to add, and a follow-up patch for `ldrex`/`strex`.

I think you should send the PSA about `shouldExpandAtomicInIR` before committing.

================
Comment at: include/llvm/Target/TargetLowering.h:1000
@@ -987,2 +999,3 @@
+  }
 
   //===--------------------------------------------------------------------===//
----------------
morisset wrote:
> jfb wrote:
> > Why no `shouldExpandAtomicCmpXchgInIR`?
> Because they should always be expanded when the target supports ll/sc and never when it supports CAS natively. I do not know of any target offering both ll/sc and cas.
> 
> The current code assumes that the target has LL/SC, this assumption is removed in another patch that I am hoping to send soon, that makes the X86 backend use this pass. This other patch will add a hasLoadLinkedStoreConditional() target hook which will both decide whether to expand CmpXchgs, and how to expand RMWs.
Could you add a temporary comment explaining this (and remove it in the next patch)? It'll make it easier for people looking at diff history to understand how the code evolved (instead of having to go to the review).

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:11065
@@ +11064,3 @@
+// things go wrong. Cortex M doesn't have ldrexd/strexd though, so don't emit
+// anything for those.
+bool ARMTargetLowering::shouldExpandAtomicStoreInIR(StoreInst *SI) const {
----------------
morisset wrote:
> jfb wrote:
> > I'm not sure that's quite correct on CortexA: Thumb only has `ldrex`/`strex` on ARMv7, and ARM has them on ARMv6K and ARMv7.
> > 
> > A further enhancement (doesn't affect correctness): processors can also use `ldrd`/`strd` atomically when `HaveLPAE` is true. On ARM's own processors this requires A15 (I'm not sure about other vendors).
> I merely kept the current behaviour and explanation. Is it ok to keep it as-is for now, and look at improving this in a later patch?
Agreed that your change is keeping the same behavior. Agreed you can fix & improve in a separate patch.

http://reviews.llvm.org/D5035






More information about the llvm-commits mailing list