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.
Details
Diff Detail
Event Timeline
This also will allow eliminating all ptrtoints in the case where the atomic is sufficiently aligned in a future patch
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? |
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?
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.
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)
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.
llvm/lib/CodeGen/AtomicExpandPass.cpp | ||
---|---|---|
707 | Yes, but when opaque pointers are removed, how will we track down all the unnecessary CreateBitCast calls? |
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 "if" seems extraneous -- Builder.CreateBitCast is already a no-op internally if it's asked to cast between opaque pointers, right?