[PATCH] D116375: [X86] Use bit test instructions to optimize some logic atomic operations

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30148
+  Instruction *I = AI->user_back();
+  if (!C1 || !AI->hasOneUse() || I->getOpcode() != Instruction::And)
+    return AtomicExpansionKind::CmpXChg;
----------------
Probably want to make sure I is in the same BB given the SelectionDAG restrictions?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30157
+  if (AI->getOperation() == AtomicRMWInst::And)
+    return (C1->getValue() ^ APInt(Bits, (uint64_t)-1)) == C2->getValue()
+               ? AtomicExpansionKind::None
----------------
Is this the same as `~C1->getValue() == C2->getValue()`?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30895
+        uint64_t Imm = cast<ConstantSDNode>(RHS)->getZExtValue();
+        if (!UI->hasOneUse() || UI->getOpcode() != ISD::AND ||
+            RHS != UI->getOperand(1) || !isPowerOf2_64(Imm))
----------------
Why does it matter if the AND has more than one use? Or were you trying to check that the ATOMIC_LOAD has only one use of the Result 0?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30931
+    Chain = Res.getValue(1);
+    Res = DAG.getNode(X86ISD::SETCC, DL, MVT::i8,
+                      DAG.getTargetConstant(X86::COND_B, DL, MVT::i8), Res);
----------------
Use the `getSETCC` function from this file.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30933
+                      DAG.getTargetConstant(X86::COND_B, DL, MVT::i8), Res);
+    Res = DAG.getAnyExtOrTrunc(Res, DL, VT);
+    if (NewImm)
----------------
Don't you need `getZExtOrTrunc` here? You aren't guaranteed to shift out the extended bits so you need to make sure they are 0 like the and would have done.


================
Comment at: llvm/test/CodeGen/X86/atomic-bit-test.ll:292
+entry:
+  %0 = atomicrmw and i16* @v16, i16 -1 monotonic, align 2
+  %and = and i16 %0, 1
----------------
Was -1 supposed to be -2?


================
Comment at: llvm/test/CodeGen/X86/atomic-bit-test.ll:332
+entry:
+  %0 = atomicrmw and i16* @v16, i16 -2 monotonic, align 2
+  %and = and i16 %0, 2
----------------
Was -2 supposed to be -3?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116375