[PATCH] Mutate TargetLowering::shouldExpandAtomicRMWInIR to specifically dictate how AtomicRMWInsts are expanded.

[JF Bastien]

In http://reviews.llvm.org/D7713#125473, @DiamondLovesYou wrote:

> In http://reviews.llvm.org/D7713#125228, @t.p.northover wrote:
>
> > No tests?
>
>
> It seems to me that the test would be passing existing tests, considering this patch happens only at an API boundary and doesn't change the behaviour of existing code.


You change description should explain that this change doesn't lead to a functional change, so the existing tests are all still valid.


REPOSITORY
  rL LLVM

================
Comment at: include/llvm/Target/TargetLowering.h:128
@@ +127,3 @@
+  enum class AtomicRMWExpansionKind {
+    Native,    // Don't expand the instruction.
+    LLSC,      // Expand the instruction into loadlinked/storeconditional.
----------------
I would just call this `None` instead of `Native`.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:143
@@ -141,1 +142,3 @@
+      }
+      MadeChange |= expandAtomicRMW(RMWI);
     } else if (CASI && TLI->hasLoadLinkedStoreConditional()) {
----------------
I find the logic with `continue` confusing, could you instead do:

```
if (...) {
  MadeChange = true;
} else {
  MadeChange |= ...;
}
```

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:229
@@ -225,2 +228,3 @@
 
 bool AtomicExpand::expandAtomicRMW(AtomicRMWInst *AI) {
+  const auto How = TLI->shouldExpandAtomicRMWInIR(AI);
----------------
I'd rename to `tryExpandAtomicRMW` since it doesn't expand them unconditionally anymore.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:231
@@ +230,3 @@
+  const auto How = TLI->shouldExpandAtomicRMWInIR(AI);
+  switch(How) {
+    case TargetLoweringBase::AtomicRMWExpansionKind::Native: return false;
----------------
`clang-format`.

I'd also just do:

```
switch (TLI->shouldExpandAtomicRMWInIR(AI)) {
```

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:237
@@ +236,3 @@
+              load-linked/store-conditional combos, but such instruction aren't \
+              supported");
+
----------------
Use string concatenation instead of backslashes:

```
  "foo "
  "bar"
```

Is equivalent to:

``` "foo bar"```

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:243
@@ +242,3 @@
+      return expandAtomicRMWToCmpXchg(AI);
+    }
+  };
----------------
Could you explain the rationale for splitting `LLSC` from `CmpXhg` instead of using the same logic as before? It does provide more flexibility, but the enum doesn't really explain why that flexibility is desirable, and doesn't point out that `LLSC` implies `hasLoadLinkedStoreConditional`: even if your `assert` checks this, it's a tricky gotcha for a Target implementer.

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8782
@@ -8778,1 +8781,3 @@
+    return TargetLoweringBase::AtomicRMWExpansionKind::Native;
+  }
 }
----------------
I think it's generally more idiomatic to do:

```
return Size <= 128 ? LLSC : Native;
```

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:11110
@@ -11106,1 +11109,3 @@
+    return TargetLoweringBase::AtomicRMWExpansionKind::Native;
+  }
 }
----------------
Same on ternary operator.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:19622
@@ -19618,1 +19621,3 @@
+      return TargetLoweringBase::AtomicRMWExpansionKind::Native;
+  }
 
----------------
Same.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:19641
@@ -19634,1 +19640,3 @@
+    else
+      return TargetLoweringBase::AtomicRMWExpansionKind::Native;
   case AtomicRMWInst::Nand:
----------------
Same.

http://reviews.llvm.org/D7713

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/