[PATCH] D124728: Allow pointer types for atomicrmw xchg

[James Y Knight via Phabricator via llvm-commits]

jyknight 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:
> > > 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`?
> > 
> > but it breaks floating point AArch64 tests 
> 
> I assume what happens here is that we don't expand the operation, the SelectionDAG lowers it to a floating-point atomic operation, then it explodes because we don't have legalization support for that?  I guess the current patch doesn't make that situation any worse.
> 
> > widenPartwordAtomicRMW is called only when the op is or, and, or xor
> 
> Wasn't really thinking about that part; sure, I guess we don't need to worry about that.
Sorry to chime in so late -- but I think the _original_ version you had, doing the coercion to integer here, unconditionally, was better.

While we support cmpxchg of pointer args emitted by users, we coerce all cmpxchg to integer types right below. And that's required -- none of the non-trivial lowerings of cmpxchg do not support non-integer values.

So, I really don't think there's value to trying to preserve non-integer types for cmpxchg as emitted by a atomicrmw xchg lowering: coerce to integer unconditionally, and be done with it.


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

https://reviews.llvm.org/D124728