[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 marked an inline comment as done.
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:
> 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: {
>       ... ...
>     }
> ```
> ```
>     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: {
>       ... ...
>     }
> ```

Done, for some reason had thought they where in different cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140939