Page MenuHomePhabricator

[InferAddressSpaces] Handle the pair of `ptrtoint`/`inttoptr`.
ClosedPublic

Authored by hliao on Jun 16 2020, 7:12 AM.

Details

Summary
  • ptrtoint and inttoptr are defined as no-op casts if the integer value as the same size as the pointer value. The pair of ptrtoint/inttoptr is in fact a no-op cast sequence between different address spaces. Teach infer-address-spaces to handle them like a bitcast.

SROA part is split into https://reviews.llvm.org/D81943

Diff Detail

Event Timeline

hliao created this revision.Jun 16 2020, 7:12 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 16 2020, 7:12 AM

This should be two separate patches - inferaddressspace and SROA.

Should be separate patches for each pass

llvm/test/Transforms/InferAddressSpaces/noop-ptrint-pair.ll
17 ↗(On Diff #271084)

This is missing a number of test cases I would like to see. First, I would like to make sure if the intermediate size is larger or smaller than the pointer size, this isn't done. I also want to stress the full combination of cases in a constant expression, not just instructions.

hliao added a comment.Jun 16 2020, 8:07 AM

This should be two separate patches - inferaddressspace and SROA.

Yes, I prepared that into 2 commits but arc combines them together.

hliao retitled this revision from [SROA] Teach SROA to perform no-op pointer conversion. [InferAddressSpaces] Handle the pair of `ptrtoint`/`inttoptr`. to [InferAddressSpaces] Handle the pair of `ptrtoint`/`inttoptr`..Jun 16 2020, 9:03 AM
hliao edited the summary of this revision. (Show Details)
tra added a subscriber: tra.Jun 16 2020, 9:15 AM

This should be two separate patches - inferaddressspace and SROA.

Yes, I prepared that into 2 commits but arc combines them together.

I usually put the patches into separate branches, with one having the other one as an upstream branch and then do arc diff with each branch checked out.
The downside is that you will need to rebase infer-branch every time you update sroa-branch.
Another approach that may work is to set arc config to only consider the checked out commit only. arc set-config base "arc:this, arc:prompt" should do that.

hliao updated this revision to Diff 271162.Jun 16 2020, 12:12 PM

Fix constant expression handling.

In D81938#2095982, @tra wrote:

This should be two separate patches - inferaddressspace and SROA.

Yes, I prepared that into 2 commits but arc combines them together.

I usually put the patches into separate branches, with one having the other one as an upstream branch and then do arc diff with each branch checked out.
The downside is that you will need to rebase infer-branch every time you update sroa-branch.
Another approach that may work is to set arc config to only consider the checked out commit only. arc set-config base "arc:this, arc:prompt" should do that.

Thanks for the ar option approach. I usually do the same thing creating a bunch of local branches to separate individual changes. It's quite cumbersome to maintain them. More than 80 local branches are really difficult to maintain. Will try that arc option.

I'm not entirely convinced this is safe in all contexts. I think you can argue that this is safe if it directly feeds a memory instruction, as the access would be undefined if it weren't valid to do the no-op cast. However, I'm not sure if this is safe if used purely in arithmetic contexts. If you're just comparing the reinterpreted pointer values for example, I don't think that would be undefined

hliao added a comment.Jun 16 2020, 2:13 PM

I'm not entirely convinced this is safe in all contexts. I think you can argue that this is safe if it directly feeds a memory instruction, as the access would be undefined if it weren't valid to do the no-op cast. However, I'm not sure if this is safe if used purely in arithmetic contexts. If you're just comparing the reinterpreted pointer values for example, I don't think that would be undefined

Would it be safe if we double-check the target hook and ensure that's a no-op addrspacecast between address spaces?

I'm not entirely convinced this is safe in all contexts. I think you can argue that this is safe if it directly feeds a memory instruction, as the access would be undefined if it weren't valid to do the no-op cast. However, I'm not sure if this is safe if used purely in arithmetic contexts. If you're just comparing the reinterpreted pointer values for example, I don't think that would be undefined

Would it be safe if we double-check the target hook and ensure that's a no-op addrspacecast between address spaces?

Yes, that would be safer since you can see there's no difference

hliao updated this revision to Diff 271551.Jun 17 2020, 7:39 PM

Add TTI hooks to double-check that address space casting is no-op.

NOTE: This change depends on - https://reviews.llvm.org/D82025 (exposing isNoopAddrSpaceCast from TLI into TTI) to be compiled. - https://reviews.llvm.org/D81943 (enhance SROA to handle no-op address space casting) to pass all testing.
hliao updated this revision to Diff 272621.Jun 22 2020, 11:12 PM

Rebase to trunk. All prerequisites are landed.

hliao added a comment.Jun 23 2020, 8:28 PM

ping for code review

arsenm added inline comments.Jun 24 2020, 7:03 AM
clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
62–64

Positive checks are preferable (here and through the rest of the file)

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
229

Can you add a comment elaborating on why this is necessary? This is special casing this to paper over the missing no-op pointer bitcast

243–245

Can you also elaborate on why the isNoopAddrSpaceCast check is needed? I'm not 100% sure it is in all contexts, so want to be sure to document that

266–267

data layout should be const reference

llvm/test/Transforms/InferAddressSpaces/noop-ptrint-pair.ll
21 ↗(On Diff #272621)

Can you also add one where the cast would be a no-op, but the integer size is different? e.g. 0 to 1 and 1 to 0 where the intermediate size is 32-bits or 128 bits

45 ↗(On Diff #272621)

Can you add some cases where the pointer value isn't used for memory? Like pure pointer arithmetic

hliao marked an inline comment as done.Jun 24 2020, 7:44 AM
hliao added inline comments.
clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
62–64

here, we need to ensure, after optimization, these values are promoted from memory

hliao updated this revision to Diff 273030.Jun 24 2020, 8:14 AM

Add comments and enhance tests.

hliao marked an inline comment as done.Jun 24 2020, 8:19 AM
hliao added inline comments.
llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
243–245

As we may convert a pair of ptr/int casts into an addrspacecast if both of them are no-op casts, if that's not a no-op addrspacecast under a specific target, we may introduce an invalid addrspacecast based on the current IR spec.

hliao updated this revision to Diff 273031.Jun 24 2020, 8:24 AM

enhance test more.

hliao marked 4 inline comments as done.Jun 24 2020, 8:36 AM
Harbormaster completed remote builds in B61552: Diff 273031.
arsenm added inline comments.Jun 24 2020, 1:17 PM
clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
62–64

Yes, you can explicitly check for the instructions you expect, rather than ones you do not

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
243–245

I mean there needs to be comment explaining all of this

hliao updated this revision to Diff 273147.Jun 24 2020, 1:40 PM

Revise again.

hliao marked 2 inline comments as done.Jun 24 2020, 1:41 PM
hliao updated this revision to Diff 273226.Jun 24 2020, 9:50 PM

Rebase to the trunk.

hliao added a comment.Jun 25 2020, 9:30 AM

ping for code review

arsenm added inline comments.Jun 25 2020, 10:02 AM
clang/test/CodeGenCUDA/amdgpu-kernel-arg-pointer-type.cu
62

Probably should use filecheck variables to avoid depending on the numbers

65

I think relying on the name may fail in a release build, filecheck variable

llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
243–245

I think this is missing one of the key points I'm worried about for the IR semantics.

I believe if you were to invalidly reinterpret a pointer with another address space, it would be undefined to access the invalid pointer and thus you can use this to infer. In pure arithmetic cases, I'm not sure this would hold. This check is to avoid breaking cases where it may still be valid to do something with reinterpreted pointer bits other than access memory

llvm/test/Transforms/InferAddressSpaces/noop-ptrint-pair.ll
2 ↗(On Diff #273226)

The pass early exits without TTI since there's no flat address space value, so this isn't really testing anything. We would have to add a flag for the flat value to bypass it.

arsenm added inline comments.Jun 25 2020, 1:51 PM
llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
238–239

The grammar for the first sentence is off. "Check if the pair of ptrtoint/inttoptr is a no-op cast."

246

No "an".

"if that pionter is dereferenced" would be more specific than than "pointer bits accessed"

hliao updated this revision to Diff 273524.Jun 25 2020, 2:39 PM

Revie the grammar. s/is/as/ in the first sentence.

hliao updated this revision to Diff 273531.Jun 25 2020, 3:20 PM

remove 'an'

arsenm accepted this revision.Jun 25 2020, 4:39 PM
This revision is now accepted and ready to land.Jun 25 2020, 4:39 PM
This revision was automatically updated to reflect the committed changes.