[PATCH] D124728: Allow pointer types for atomicrmw xchg

[Takafumi Arakaki via Phabricator via llvm-commits]

tkf marked an inline comment as done and an inline comment as not done.
tkf added inline comments.


================
Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:290
+            (RMWI->getValOperand()->getType()->isFloatingPointTy() ||
+             RMWI->getValOperand()->getType()->isPointerTy())) {
           // TODO: add a TLI hook to control this so that each target can
----------------
efriedma wrote:
> Can we try to preserve the pointer type where possible?  cmpxchg can take a pointer.
`emitLoadLinked` [1] and `emitStoreConditional` [2] for `AArch64TargetLowering` use `CreateBitCast` to/from integer always. So, if I understand correctly, we would need casting to integer in the TLI or somewhere. Looking at https://reviews.llvm.org/D103232 that added this for floating point types, doing this early and unconditionally seems to be intentional. Or is the situation different in pointer types compared to floating point types?

[1]: https://github.com/llvm/llvm-project/blob/eeccdd318d25f68745cdf8199f7e8c251d0cf65e/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L19710
[2]: https://github.com/llvm/llvm-project/blob/eeccdd318d25f68745cdf8199f7e8c251d0cf65e/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L19747



================
Comment at: llvm/lib/IR/Verifier.cpp:3929
+    Check(ElTy->isIntegerTy() || ElTy->isFloatingPointTy() ||
+              ElTy->isPointerTy(),
           "atomicrmw " + AtomicRMWInst::getOperationName(Op) +
----------------
efriedma wrote:
> Is the verifier missing a check for the power-of-two constraint that the IR parser enforces?
It looks like `visitAtomicRMWInst` [calls](https://github.com/llvm/llvm-project/blob/eeccdd318d25f68745cdf8199f7e8c251d0cf65e/llvm/lib/IR/Verifier.cpp#L3943) `checkAtomicMemAccessSize` which [checks the power-of-two size constraint](https://github.com/llvm/llvm-project/blob/eeccdd318d25f68745cdf8199f7e8c251d0cf65e/llvm/lib/IR/Verifier.cpp#L3802-L3803).



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

https://reviews.llvm.org/D124728