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

Mon Feb 21 18:00:31 PST 2022

craig.topper added inline comments.

================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:5451
+      Info.align = Align(Size);
+      Info.flags |= MachineMemOperand::MOLoad | MachineMemOperand::MOStore;
+      return true;
----------------
pengfei wrote:
> craig.topper wrote:
> > pengfei wrote:
> > > craig.topper wrote:
> > > > Does this need MOVolatile?
> > > Maybe not. `MOVolatile` is usually set when `isVolatile()` return true in load and store instructions. We don't have such info in a target intrinsic.
> > > But seems we never worried about it, and there're few place it is set in X86 code. So I guess X86 is fine with or without `MOVolatile`?
> > This information is used to create a MachineMemOperand. I think that MachineMemOperand needs to know this access can't undergo certain optimizations because it represents an atomic access. I don't think this interface can create an atomic MachineMemOperand, but it can create a volatile one.
> `volatile` is an memory attribute which is not atomic exclusive. https://godbolt.org/z/1o9Y7j5xe
> We have ignored it on all existing target memory intrinsics, so I think we don't need any special handling here.
Do the LOCK_BTS/BTR/BTC show up in MIR with "monotonic"(or other atomic ordering) in their memory operand printing in MIR? If not you have a bug waiting to happen. I think "volatile" is stronger than any of the atomic orderings. So it works in place of them.

RISCV uses MOVolatile in getTgtMemIntrinsic for riscv_masked_atomicrmw*

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120199