This is an archive of the discontinued LLVM Phabricator instance.

AtomicExpand: Use llvm.ptrmask instead of ptrtoint
ClosedPublic

Authored by arsenm on Sep 20 2022, 1:27 PM.

Details

Summary

This removes the ptrtoint from the load's pointer operand, although we
can't entirely eliminate these to get the LSB shift. In a future
patch, this will avoid ptrtoint in the case where the atomic is
overaligned to the word size.

Diff Detail

Build Status
Buildable 188424

Event Timeline

arsenm created this revision.Sep 20 2022, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 1:27 PM
arsenm requested review of this revision.Sep 20 2022, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 1:27 PM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm updated this revision to Diff 461683.Sep 20 2022, 1:33 PM

VE test update

This also will allow eliminating all ptrtoints in the case where the atomic is sufficiently aligned in a future patch

jyknight added a comment.EditedSep 20 2022, 3:26 PM

Can you explain what effect you expect this to have? It removes all the inttoptr -- maybe that's useful in itself?

Is the remaining ptrtoint to get the low bits harmful? Being able to omit the ptrtoint when the value is sufficiently aligned seems likely to be nearly useless -- this code is only used when the width of the atomic operation requested is smaller than the smallest size of atomic supported by the hardware. And sure, the smaller value could be overaligned in some cases, but...

llvm/lib/CodeGen/AtomicExpandPass.cpp
707

This "if" seems extraneous -- Builder.CreateBitCast is already a no-op internally if it's asked to cast between opaque pointers, right?

Can you explain what effect you expect this to have? It removes all the inttoptr -- maybe that's useful in itself?

Moving towards the goal of never having compiler introduced inttoptr

Can you explain what effect you expect this to have? It removes all the inttoptr -- maybe that's useful in itself?

Moving towards the goal of never having compiler introduced inttoptr

That was sort of my guess -- but, then, this change already DOES fix (this part of) the problem, right? We have a ptrtoint remaining, but not an inttoptr -- that should be fine?

Can you explain what effect you expect this to have? It removes all the inttoptr -- maybe that's useful in itself?

Moving towards the goal of never having compiler introduced inttoptr

That was sort of my guess -- but, then, this change already DOES fix (this part of) the problem, right? We have a ptrtoint remaining, but not an inttoptr -- that should be fine?

It half fixes it. It's still introducing an inttoptr in order to get the low bits. We could introduce a second ptrmask with the inverted mask, but we still need to inttoptr in order to shift that into the Inv_Mask position

Can you explain what effect you expect this to have? It removes all the inttoptr -- maybe that's useful in itself?

Moving towards the goal of never having compiler introduced inttoptr

That was sort of my guess -- but, then, this change already DOES fix (this part of) the problem, right? We have a ptrtoint remaining, but not an inttoptr -- that should be fine?

It half fixes it. It's still introducing an inttoptr in order to get the low bits. We could introduce a second ptrmask with the inverted mask, but we still need to inttoptr in order to shift that into the Inv_Mask position

OK, now I'm confused again...

My understanding:.

  • converting from integer to pointer ("inttoptr") is undesirable to introduce in transformations. (Such instruciton may be present in the original input, but shouldn't be added if it was not.)
  • converting from pointer to integer ("ptrtoint") is OK.

In the current AtomicExpandPass.cpp, we have 3 mentions of CreateIntToPtr:

  • convertAtomicXchgToIntegerType
  • convertCmpXchgToIntegerType
  • createMaskInstrs

The first two are about pointer-valued atomicrmw and cmpxchg operands, which are unrelated to this change, so we'll ignore that. The third is removed by this patch.

So, ISTM this patch does entirely fix the problem it sets out to fix -- it has removed the only CreateIntToPtr from the address computation. There remains a CreatePtrToInt to extract the value shift/mask -- but that's not a problem. Right?

So, ISTM this patch does entirely fix the problem it sets out to fix -- it has removed the only CreateIntToPtr from the address computation. There remains a CreatePtrToInt to extract the value shift/mask -- but that's not a problem. Right?

We don't want inttoptr or ptrtoint anywhere, since they still are capturing, but this is the limit of what is possible with current IR. We could push one of the ptrtoints later after doing the mask to get the LSB with the intrinsic, but you still need the ptrtoint to do the shift

We don't want inttoptr or ptrtoint anywhere, since they still are capturing, but this is the limit of what is possible with current IR. We could push one of the ptrtoints later after doing the mask to get the LSB with the intrinsic, but you still need the ptrtoint to do the shift

OK -- it sounds like the root of the confusion here is that

My understanding:

  • converting from integer to pointer ("inttoptr") is undesirable to introduce in transformations. (Such instruciton may be present in the original input, but shouldn't be added if it was not.)
  • converting from pointer to integer ("ptrtoint") is OK.

is incorrect. Sorry to be a pain, but do you have a link that describes the issues here? I may well not be remembering properly, but I had thought previous discussions and changes were about pointer provenance issues that arise from the inttoptr direction.

So I'm afraid I'm not at all clear to me why the ptrtoint is problematic -- and in particular, why you wrote in the change description that this might not actually be worth doing since we cannot remove the ptrtoint.

is incorrect. Sorry to be a pain, but do you have a link that describes the issues here? I may well not be remembering properly, but I had thought previous discussions and changes were about pointer provenance issues that arise from the inttoptr direction.

ptrtoint is the most basic capture

So I'm afraid I'm not at all clear to me why the ptrtoint is problematic -- and in particular, why you wrote in the change description that this might not actually be worth doing since we cannot remove the ptrtoint.

This does fully eliminate all ptrtoint in the overaligned case. I'd still rather move towards reducing uses of it to the point where it's required (which moves it to the shift)

is incorrect. Sorry to be a pain, but do you have a link that describes the issues here? I may well not be remembering properly, but I had thought previous discussions and changes were about pointer provenance issues that arise from the inttoptr direction.

ptrtoint is the most basic capture

Also, just see the langref description for llvm.ptrmask

is incorrect. Sorry to be a pain, but do you have a link that describes the issues here? I may well not be remembering properly, but I had thought previous discussions and changes were about pointer provenance issues that arise from the inttoptr direction.

ptrtoint is the most basic capture

OK.

Also, just see the langref description for llvm.ptrmask

I looked at that first thing. It does not -- at least, not obviously -- discuss this side of the issue. It describes its semantics as being equivalent to getelementptr ptr, (ptrtoint(ptr) & mask) - ptrtoint(ptr). It also says it preserves more information about the resulting pointer than ptrtoint+inttoptr. Both these statements imply that the issue it's solving is the loss of information that occurs via creation of a new pointer with inttoptr -- and that ptrmask exists primarily because it's a much simpler canonical form to deal with than the equivalent GEP -- which requires extraneous addition/subtraction operations.

Anyways, I have no issue with this change itself (only the 1 minor nit) -- just with the description.

arsenm added inline comments.Sep 23 2022, 9:01 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
707

Yes, but when opaque pointers are removed, how will we track down all the unnecessary CreateBitCast calls?

arsenm updated this revision to Diff 462512.Sep 23 2022, 9:03 AM
arsenm edited the summary of this revision. (Show Details)

Remove opaque pointer check

jyknight accepted this revision.Sep 23 2022, 9:52 AM

LGTM, thanks for the discussion.

I'd appreciate if you modify the commit to have a clearer description before pushing.

llvm/lib/CodeGen/AtomicExpandPass.cpp
707

I'd start by looking at all the calls to Type::getPointerTo(AS), Type::get*PtrTy(...), PointerType::get(Ty, AS), etc. Many such calls can probably be removed entirely, along with their use, being used only as input to some cast creation. Others would still be required, and changed to PointerType::get(Ctx, AS) instead. I suspect that'll get rid of most of the redundant casts as a side-effect.

Then, I'd make a temporary local modification where attempting a no-op cast from ptr to ptr will assert-fail, run tests, and examine all the failure locations, to see which other casts ought to be removed.

This revision is now accepted and ready to land.Sep 23 2022, 9:52 AM

LGTM, thanks for the discussion.

I'd appreciate if you modify the commit to have a clearer description before pushing.

I just modified it with the last revision