[PATCH] D124728: Allow pointer types for atomicrmw xchg

[Takafumi Arakaki via Phabricator via llvm-commits]

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:
> tkf wrote:
> > 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
> > 
> Unlike floats, pointers are allowed as operands to cmpxchg.  So if we end up in insertRMWCmpXchgLoop, we shouldn't need to ptrtoint/inttoptr to force the type of the cmpxchg to an integer type.
> 
> Wasn't thinking about the other codepaths for lowering atomicrmw.  I guess some of them require integers; maybe some could be modified in the future.
> 
> Hopefully it's not too hard to move this code specifically to the places that need it?  If that's tricky for some reason, we can skip it for now, I guess.
I added `TLI->shouldConvertAtomicRMWToIntegerType(RMWI)` to keep the pointer type in x86 (and resolve the TODO comment just below in the source code). Is this a valid solution?



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

https://reviews.llvm.org/D124728