[PATCH] D124728: Allow pointer types for atomicrmw xchg

[Takafumi Arakaki via Phabricator via llvm-commits]

tkf added a comment.

In D124728#3506193 <https://reviews.llvm.org/D124728#3506193>, @asb wrote:

> I think specifically, atomicrmw of non-integral pointer types may have to be disallowed?

In Julia, we use non-integral pointer types for GC-tracked objects. Using xchg for such pointers is the primary motivation for this patch. We would like to keep non-integral pointer types in our lowering stages while maintaining xchg instructions.



================
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:
> > > 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.
I tried to make something you suggested. It calls `convertAtomicXchgToIntegerType` in `tryExpandAtomicRMW`. It avoids `convertAtomicXchgToIntegerType` on `AtomicExpansionKind::CmpXChg` if the value operand is pointer.

I initially tried it only for LL/SC but it breaks floating point AArch64 tests on `LegalizeDAG`. I wonder if that's why https://reviews.llvm.org/D103232 introduced the conversion that is triggered in many cases?

> move the convertAtomicXchgToIntegerType() call into tryExpandAtomicRMW()/widenPartwordAtomicRMW()

`widenPartwordAtomicRMW` is called only when the op is `or`, `and`, or `xor` which are not supported on pointers and floats. So, I don't think we need to call it before `widenPartwordAtomicRMW`?



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

https://reviews.llvm.org/D124728