[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);
----------------
goldstein.w.n wrote:
> pengfei wrote:
> > goldstein.w.n wrote:
> > > 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
> > > ```
> > I mean split `x86_atomic_bt*_rm` from it and then change it to `I.getArgOperand(1)->getType()->getScalarSizeInBits()`
> > Other intrinsics are correct to use `I.getType()->getScalarSizeInBits()`.
> > I mean split `x86_atomic_bt*_rm` from it and then change it to `I.getArgOperand(1)->getType()->getScalarSizeInBits()`
> > Other intrinsics are correct to use `I.getType()->getScalarSizeInBits()`.
> 
> What does `"split `x86_atomic_bt*_rm` from it" mean?
```
    case Intrinsic::x86_cmpccxadd32:
    case Intrinsic::x86_cmpccxadd64:
    case Intrinsic::x86_atomic_bts:
    case Intrinsic::x86_atomic_btc:
    case Intrinsic::x86_atomic_btr: {
      ... ...
    }
    case Intrinsic::x86_atomic_bts_rm:
    case Intrinsic::x86_atomic_btc_rm:
    case Intrinsic::x86_atomic_btr_rm: {
      ... ...
    }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140939