[PATCH] D124728: Allow pointer types for atomicrmw xchg

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

> One thing that's not immediately clear to me is what the the appropriate handling of pointers with a non-default address space should be? I think specifically, atomicrmw of non-integral pointer types may have to be disallowed?

I don't see a particular problem here?  I mean, an atomic xchg is perfectly sensible for non-integral pointers; semantically, it doesn't even require examining the pointer bits.  I guess it's possible some targets can't implement an atomic pointer xchg, but it's not like we're forcing frontends to use it...



================
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
----------------
tkf wrote:
> 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?
> 
I think we want to move the convertAtomicXchgToIntegerType() call into tryExpandAtomicRMW()/widenPartwordAtomicRMW(); at that point, we can tell whether we need to force a conversion to an integer type, without explicitly asking the target, based on the IR instructions we're going to generate.

We don't really want to make targets implement hooks to compute things we already know.


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

https://reviews.llvm.org/D124728