[PATCH] D140939: [X86] Transform AtomicRMW logic operations to BT{R|C|S} if only changing/testing a single bit.

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:5653
       Info.ptrVal = I.getArgOperand(0);
       unsigned Size = I.getType()->getScalarSizeInBits();
       Info.memVT = EVT::getIntegerVT(I.getType()->getContext(), Size);
----------------
Should the `Size` be same with operand 1 rather than result?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31434
+  if (C) {
+    // Check if V is a power of 2 or or NOT power of 2.
+    if (isPowerOf2_64(C->getZExtValue())) {
----------------
or


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31435
+    // Check if V is a power of 2 or or NOT power of 2.
+    if (isPowerOf2_64(C->getZExtValue())) {
+      BTK = ConstantBit;
----------------
No parentheses needed for single line scope.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31453
+      auto *OpC1 = dyn_cast<ConstantInt>(I->getOperand(1));
+      // Check if this is a NOT instruction: -1 - X or X/-1 ^ -1/X
+      if (!OpC0 && (!OpC1 || I->getOpcode() == Instruction::Sub))
----------------
Should it be a canonical form that we can assume it is always `X ^ -1`?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31454
+      // Check if this is a NOT instruction: -1 - X or X/-1 ^ -1/X
+      if (!OpC0 && (!OpC1 || I->getOpcode() == Instruction::Sub))
+        return {nullptr, UndefBit};
----------------
We can use `match(....)` for better readability?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31527
+  auto BitChange = FindSingleBitChange(AI->getValOperand());
+  if (!BitChange.first || BitChange.second == UndefBit || !AI->hasOneUse() ||
+      I->getOpcode() != Instruction::And ||
----------------
Is enough to only check `BitChange.second == UndefBit`?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31559
+
+  // If shift amounts are not the same we can't use BitTestIntrinsic
+  if (BitChange.first != BitTested.first)
----------------
Missing the last period `.`


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31564
+  // If atomic AND need to be masking all be one bit and testing the one bit
+  // unset in the mask
   if (AI->getOperation() == AtomicRMWInst::And)
----------------
ditto.


================
Comment at: llvm/test/CodeGen/X86/atomic-rm-bit-test.ll:4622-4628
+; X86-NEXT:    jae .LBB78_1
+; X86-NEXT:  # %bb.2: # %if.then
+; X86-NEXT:    movl (%ecx,%eax,4), %eax
+; X86-NEXT:    retl
+; X86-NEXT:  .LBB78_1:
 ; X86-NEXT:    movl $123, %eax
 ; X86-NEXT:    retl
----------------
The branch code doesn't look necessary. Can we necessary it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140939