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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n 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);
----------------
pengfei wrote:
> Should the `Size` be same with operand 1 rather than result?
> Should the `Size` be same with operand 1 rather than result?

If I change it I run into assertion errors.

`I.getArgOperand(0)->getType()->getScalarSizeInBits()` the `Size` can be zero (bitwidth too small)

or `I.getArgOperand(1)->getType()->getScalarSizeInBits()` I run into:
```
llc: /home/noah/programs/opensource/llvm-dev/src/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:11108: llvm::MemSDNode::MemSDNode(unsigned int, unsigned int, const llvm::DebugLoc&, llvm::SDVTList, llvm::EVT, llvm::MachineMemOperand*): Assertion `memvt.getStoreSize().getKnownMinSize() <= MMO->getSize() && "Size mismatch!"' failed.
```

Maybe that means there is a bug in the patch elsewhere?

Running the test here are the sizes I see:
```
Sizes: 8 / 0 / 16
Sizes: 8 / 0 / 16
Sizes: 8 / 0 / 16
Sizes: 8 / 0 / 16
Sizes: 16 / 0 / 8
Sizes: 16 / 0 / 8
Sizes: 16 / 0 / 8
Sizes: 16 / 0 / 8
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 32 / 0 / 8
Sizes: 32 / 0 / 8
Sizes: 32 / 0 / 8
Sizes: 32 / 0 / 8
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 64 / 0 / 8
Sizes: 64 / 0 / 8
Sizes: 64 / 0 / 8
```


================
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))
----------------
pengfei wrote:
> Should it be a canonical form that we can assume it is always `X ^ -1`?
> Should it be a canonical form that we can assume it is always `X ^ -1`?

Probably defacto fixed by using `match` (didn't know `match` existed when I wrote this patch :/ alot to learn).


================
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
----------------
pengfei wrote:
> The branch code doesn't look necessary. Can we necessary it?
> The branch code doesn't look necessary. Can we necessary it?

I think it is b.c we don't `cmovcc` loads.

For
```
if.then:                                          ; preds = %entry
  %idxprom = zext i32 %c to i64
  %arrayidx = getelementptr inbounds i32, ptr %v, i64 %idxprom
  %1 = load i32, ptr %arrayidx, align 4
  br label %return
```

And
```
return:                                           ; preds = %entry, %if.then
  %retval.0 = phi i32 [ %1, %if.then ], [ 123, %entry ]
  ret i32 %retval.0
```

a branch seems correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140939