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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsX86.td:67
+let TargetPrefix = "x86" in {
+  def int_x86_bittest :
+              Intrinsic<[llvm_anyint_ty], [llvm_ptr_ty, llvm_i8_ty, llvm_i32_ty],
----------------
Probably better as int_x86_atomic_bitttest?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:5450
+      Info.memVT = EVT::getIntegerVT(I.getType()->getContext(), Size);
+      Info.align = Align(Size);
+      Info.flags |= MachineMemOperand::MOLoad | MachineMemOperand::MOStore;
----------------
Does something check natural alignment before we create the intrinsic?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:5451
+      Info.align = Align(Size);
+      Info.flags |= MachineMemOperand::MOLoad | MachineMemOperand::MOStore;
+      return true;
----------------
Does this need MOVolatile?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30509
+  Instruction *I = AI->user_back();
+  LLVMContext &Ctx = AI->getParent()->getParent()->getContext();
+  unsigned Imm =
----------------
Does `AI->getContext()` work?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30515
+  Value *Addr =
+      Builder.CreateBitCast(AI->getPointerOperand(), Type::getInt8PtrTy(Ctx));
+  Value *Result = Builder.CreateCall(
----------------
CreateBitCast -> createPointerCast


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30515
+  Value *Addr =
+      Builder.CreateBitCast(AI->getPointerOperand(), Type::getInt8PtrTy(Ctx));
+  Value *Result = Builder.CreateCall(
----------------
craig.topper wrote:
> CreateBitCast -> createPointerCast
Probably doesn't really matter, but you should pass the address space to getInt8PtrTy.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30517
+  Value *Result = Builder.CreateCall(
+      BitTest, {Addr, ConstantInt::get(Type::getInt8Ty(Ctx), Imm),
+                ConstantInt::get(Type::getInt32Ty(Ctx), SCR)});
----------------
ConstantInt::get(Type::getInt8Ty(Ctx), Imm) -> Builder.getInt8(Imm)


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:30518
+      BitTest, {Addr, ConstantInt::get(Type::getInt8Ty(Ctx), Imm),
+                ConstantInt::get(Type::getInt32Ty(Ctx), SCR)});
+  I->replaceAllUsesWith(Result);
----------------
Builder.getInt32(SCR)


================
Comment at: llvm/lib/Target/X86/X86InstrCompiler.td:847
+                                    SDTCisVT<4, i32>]>,
+               [SDNPHasChain, SDNPMayStore, SDNPMemOperand]>;
+// We use 0/1/2 in SCR to represent BTS/BTC/BTR.
----------------
Why no SDNPMayLoad?


================
Comment at: llvm/lib/Target/X86/X86InstrCompiler.td:848
+               [SDNPHasChain, SDNPMayStore, SDNPMemOperand]>;
+// We use 0/1/2 in SCR to represent BTS/BTC/BTR.
+// It matches with the selection in emitBitTestAtomicRMWIntrinsic.
----------------
Can we use 3 separate intrinsics? I'm not sure this made up encoding is saving much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120199