[PATCH] Add a thread-model knob for lowering atomics on baremetal & single threaded systems

Robin Morisset morisset at google.com
Wed Aug 20 13:19:32 PDT 2014


A few inline comments.

I would also suggest tests for atomic loads/stores/fences and not just atomicRMW.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:752
@@ -751,6 +751,3 @@
   // ARMv6 Thumb1 (except for CPUs that support dmb / dsb) and earlier use
-  // the default expansion.
-  if (Subtarget->hasAnyDataBarrier() && !Subtarget->isThumb1Only()) {
-    // ATOMIC_FENCE needs custom lowering; the others should have been expanded
-    // to ldrex/strex loops already.
-    setOperationAction(ISD::ATOMIC_FENCE,     MVT::Other, Custom);
+  // the default expansion. If we are targeting a single threded system,
+  // then set them all for expand so we can lower them later into their
----------------
single threded -> single threaded

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:757
@@ +756,3 @@
+      TM.Options.ThreadModel == ThreadModel::Single) {
+    if (TM.Options.ThreadModel == ThreadModel::Single) {
+      setOperationAction(ISD::ATOMIC_FENCE,   MVT::Other, Expand);
----------------
This repetition of the test for ThreadModel::Single looks unnecessary to me.

Why not test for it first, then put the rest in an else block ?


```
if (TM.Options>ThreadModel == ThreadModel::Single) {
  setOperationAction(ISD::ATOMIC_FENCE,  MVT::Other, Expand);
else if (Subtarget->hasAnyDataBarrier() && !Subtarget->isThumb1Only()) {
...
```

As a bonus, it makes the logic simpler, avoiding setting insertFencesForAtomic (which is a no-op in that case I believe, since all atomic operations will have been lowered before).

================
Comment at: lib/Target/ARM/ARMTargetMachine.cpp:161
@@ -160,3 +160,3 @@
 void ARMPassConfig::addIRPasses() {
   addPass(createAtomicExpandLoadLinkedPass(TM));
 
----------------
Shouldn't this pass be disabled when the ThreadModel is single? Especially as it runs first, I expect it will turn for example atomicrmw add in a loop of ldrex/strex instead of a simple add instruction (whenever the subtarget does not disable it at least).

http://reviews.llvm.org/D4984






More information about the llvm-commits mailing list