This is an archive of the discontinued LLVM Phabricator instance.

Allow pointer types for atomicrmw xchg
ClosedPublic

Authored by tkf on Apr 30 2022, 10:44 PM.

Details

Summary

This adds support for pointer types for atomic xchg and let us write instructions such as atomicrmw xchg i64** %0, i64* %1 seq_cst. This is similar to the patch for allowing atomicrmw xchg on floating point types https://reviews.llvm.org/D52416.

Motivation: In Julia, we keep GC-managed objects as pointer in LLVM IR so that our compiler pass can insert GC-related instructions to track these objects. Thus, we cannot easily use bitcast (like clang does) to use atomicrmw xchg for swapping a pointer value (https://github.com/JuliaLang/julia/issues/44932). Adding direct support for xchg of pointer in LLVM would let us use native instructions in a wider class of programs. I think it may be beneficial to other languages with GC using approaches similar to Julia.

Diff Detail

Event Timeline

tkf created this revision.Apr 30 2022, 10:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2022, 10:44 PM
tkf requested review of this revision.Apr 30 2022, 10:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2022, 10:44 PM
efriedma added inline comments.May 2 2022, 2:21 PM
llvm/lib/AsmParser/LLParser.cpp
7464

You can change this to get the size from the datalayout, instead of using getPrimitiveSizeInBits(). (Not that I really expect anyone to declare a non-power-of-two pointer type, but I don't think we actually forbid it.)

llvm/lib/CodeGen/AtomicExpandPass.cpp
288

Can we try to preserve the pointer type where possible? cmpxchg can take a pointer.

efriedma added inline comments.May 2 2022, 2:23 PM
llvm/lib/IR/Verifier.cpp
3929

Is the verifier missing a check for the power-of-two constraint that the IR parser enforces?

tkf updated this revision to Diff 426616.May 3 2022, 2:35 AM

Use datalayout for detecting pointer size

tkf marked an inline comment as done and an inline comment as not done.May 3 2022, 2:36 AM
tkf added inline comments.
llvm/lib/CodeGen/AtomicExpandPass.cpp
288

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

llvm/lib/IR/Verifier.cpp
3929

It looks like visitAtomicRMWInst calls checkAtomicMemAccessSize which checks the power-of-two size constraint.

efriedma added inline comments.May 3 2022, 9:57 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
288

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.

tkf updated this revision to Diff 426929.May 4 2022, 12:37 AM

Add TLI->shouldConvertAtomicRMWToIntegerType so that each target can opt-out convertAtomicXchgToIntegerType.

tkf updated this revision to Diff 426932.May 4 2022, 12:46 AM

Document shouldConvertAtomicRMWToIntegerType

tkf added inline comments.May 4 2022, 12:49 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
288

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?

tkf marked an inline comment as not done.May 4 2022, 1:20 AM
asb added a comment.May 11 2022, 7:19 AM

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?

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...

llvm/lib/CodeGen/AtomicExpandPass.cpp
288

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.

asb added a comment.May 12 2022, 12:47 AM

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...

My concern was cases where there's no available atomic operation that can handle the pointer (e.g. atomic operations are only available on GPRs and it can't be stored in a GPR). Though as you say, frontends can just not generate it.

tkf updated this revision to Diff 429242.May 13 2022, 7:46 AM

Move convertAtomicXchgToIntegerType to tryExpandAtomicRMW

tkf added a comment.May 13 2022, 7:50 AM

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.

llvm/lib/CodeGen/AtomicExpandPass.cpp
288

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?

efriedma added inline comments.May 13 2022, 3:59 PM
llvm/lib/CodeGen/AtomicExpandPass.cpp
288

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.

548

Do you also want to check for AtomicExpansionKind::None here?

tkf added inline comments.May 18 2022, 5:53 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
548

I saw the failure from LegalizeDAG in an AArch64 test that does atomicrmw xchg on floating point. It occurs with AtomicExpansionKind::None. This is the failure in LegalizeDAG I mentioned above. Since presumably the legalization does not support xchg of floats, we need to convert it on xchg of ints here.

I think making this part simple requires adding supports to the legalization. But I'm not familiar with that part of LLVM.

efriedma added inline comments.May 18 2022, 8:56 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
548

No, I meant if we have pointer, and the expansion kind is None. I wasn't suggesting you change what we do for floating-point ops.

tkf updated this revision to Diff 430643.May 19 2022, 4:38 AM

Don't convert pointers to integers for AtomicExpansionKind::None

tkf added inline comments.May 19 2022, 4:40 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
548

Thanks for the clarification. I exclude the AtomicExpansionKind::None case when the value operand type is a pointer.

efriedma added inline comments.May 19 2022, 8:42 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
523

Is this change necessary?

jyknight added inline comments.May 19 2022, 9:38 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
288

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.

tkf added inline comments.May 19 2022, 10:01 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
288

I'm happy to roll back to the original patch.

@efriedma What do you think?

efriedma added inline comments.May 19 2022, 10:56 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
288

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.

tkf updated this revision to Diff 430906.May 20 2022, 1:47 AM

Rollback to the version with unconditional casting to integer, as discussed in https://reviews.llvm.org/D124728#inline-1208265

Remove unneeded check of pointer type, as pointed out by https://reviews.llvm.org/D124728#inline-1208221

tkf marked an inline comment as done.May 20 2022, 1:49 AM
tkf added inline comments.
llvm/lib/CodeGen/AtomicExpandPass.cpp
288

I reverted the patch to the initial version plus some other improvements that are applicable to the initial version.

jyknight accepted this revision.May 20 2022, 9:17 AM

Thanks!

This revision is now accepted and ready to land.May 20 2022, 9:17 AM
tkf added a comment.May 24 2022, 6:03 AM

Can this patch land? Is there anything else that needs to be done?

Can this patch land? Is there anything else that needs to be done?

Yes. If you don't have permission to push it, I can do so for you. Just let me know, thanks!

lkail added a subscriber: lkail.May 24 2022, 7:14 AM
tkf added a comment.May 25 2022, 4:26 AM

Yes, please merge this. I don't have permission to push it.

This revision was landed with ongoing or failed builds.May 25 2022, 9:24 AM
Closed by commit rG18e6b8234a0d: Allow pointer types for atomicrmw xchg (authored by tkf, committed by jyknight). · Explain Why
This revision was automatically updated to reflect the committed changes.
llvm/lib/IR/Verifier.cpp