[PATCH] D142166: [X86] Make `shouldExpandLogicAtomicRMWInIR` able to match both operands.

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31534-31536
+  // This is a redundant AND, it should get cleaned up elsewhere.
+  if (AI == I->getOperand(OtherIdx))
+    return AtomicExpansionKind::CmpXChg;
----------------
pengfei wrote:
> Do you have a test for it? I suspect it's dead code. Maybe just use assert?
> Do you have a test for it? I suspect it's dead code. Maybe just use assert?

I think assert is a mistake b.c its definitely representable in IR:

```
define zeroext i16 @atomic_shl1_small_mask_xor_16_gpr_val(ptr %v, i16 zeroext %c) nounwind {
entry:
  %0 = and i16 %c, 7
  %shl = shl nuw nsw i16 1, %0
  %1 = atomicrmw xor ptr %v, i16 %shl monotonic, align 2
  %and = and i16 %1, %1
  ret i16 %and
}
``` 

I think its okay to miss optimizations when the IR is funky
but we shouldn't crash.

Added a test just to make sure we don't crash.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31540-31541
   if (BitChange.second == ConstantBit || BitChange.second == NotConstantBit) {
     auto *C1 = dyn_cast<ConstantInt>(AI->getValOperand());
     assert(C1 != nullptr);
+    auto *C2 = dyn_cast<ConstantInt>(I->getOperand(OtherIdx));
----------------
pengfei wrote:
> This can trun into `auto *C1 = cast<ConstantInt>(AI->getValOperand());`.
> This can trun into `auto *C1 = cast<ConstantInt>(AI->getValOperand());`.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142166