[PATCH] D124728: Allow pointer types for atomicrmw xchg

[Takafumi Arakaki via Phabricator via llvm-commits]

tkf marked an inline comment as 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:
> tkf wrote:
> > jyknight wrote:
> > > 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.
> > I'm happy to roll back to the original patch.
> > 
> > @efriedma What do you think?
> The idea here was is to give more flexibility to backends that use non-integral pointer types... but I missed that the code below forces cmpxchg to use an integral type.  Sure, we can revert it, and let whoever needs that look into it as a followup.
I reverted the patch to the initial version plus some other improvements that are applicable to the initial version.


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

https://reviews.llvm.org/D124728