This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold phi ( inttoptr/ptrtoint x ) to phi (x)
ClosedPublic

Authored by Krishnakariya on Jul 19 2021, 10:37 AM.

Details

Summary

The inttoptr/ptrtoint roundtrip optimization is not always correct.
We are working towards removing this optimization and adding support to specific cases where this optimization works.

In this patch, we focus on phi-node operands with inttoptr casts.
We know that ptrtoint( inttoptr( ptrtoint x) ) is same as ptrtoint (x).
So, we want to remove this roundtrip cast which goes through phi-node.

Diff Detail

Event Timeline

Krishnakariya created this revision.Jul 19 2021, 10:37 AM
Krishnakariya requested review of this revision.Jul 19 2021, 10:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2021, 10:37 AM
Krishnakariya edited the summary of this revision. (Show Details)
Krishnakariya added a reviewer: spatel.
aqjune added inline comments.Jul 26 2021, 5:29 PM
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
324

Should we exclude cases where input pointers have different types?
After opaque pointers are introduced everything will be fine - it is a safeguard that will work until then.

aqjune added inline comments.Jul 26 2021, 5:32 PM
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
324

For example:

p = phi i64 [ ptrtoint i8* %a, BB1 ], [ ptrtoint float* %b, BB2 ]
i = inttoptr p
Krishnakariya marked 2 inline comments as done.

Added a test for different input pointer types (func_pointer_different_types).

I have a question about addrspacecast: is ptrtoint(inttoptr(ptrtoint pty1 p) to pty2) equivalent to ptrtoint(addrspacecast pty1 p to pty2)?
Does anyone have idea about this?

I have a question about addrspacecast: is ptrtoint(inttoptr(ptrtoint pty1 p) to pty2) equivalent to ptrtoint(addrspacecast pty1 p to pty2)?
Does anyone have idea about this?

I'm going to guess no. inttoptr/ptrtoint are essentially no-op bitcasts,
while addrspacecast is implementation-defined and in general case can be assumed to e.g. offset the pointer.

I have a question about addrspacecast: is ptrtoint(inttoptr(ptrtoint pty1 p) to pty2) equivalent to ptrtoint(addrspacecast pty1 p to pty2)?
Does anyone have idea about this?

I'm going to guess no. inttoptr/ptrtoint are essentially no-op bitcasts,
while addrspacecast is implementation-defined and in general case can be assumed to e.g. offset the pointer.

Oh, thanks. @Krishnakariya does this patch deal with the case?

Yes, it handles this case. I have added a new test for the same.

aqjune accepted this revision.Aug 2 2021, 6:26 AM

LGTM.

This revision is now accepted and ready to land.Aug 2 2021, 6:26 AM
aqjune added a comment.Aug 2 2021, 6:36 AM

I left a few nitpicks that is probably worth addressed

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
306

You can do this folding when PN has multiple uses all of which are ptrtoint as well.

334

Is there a reason why a new phi is created?
Would replacing the corresponding op with the optimized pointer be enough?

Krishnakariya marked 2 inline comments as done.
Krishnakariya retitled this revision from [InstCombine] Fold phi ( inttoptr/ptrtoint x ) to phi (x) to [InstCombine] Fold phi ( inttoptr/ptrtoint x ) to phi (x).
Krishnakariya edited the summary of this revision. (Show Details)

Simplified code.

aqjune added inline comments.Aug 2 2021, 9:20 PM
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
307

Perhaps there was llvm::all_of that can take this loop body as lambda. Let's use it to simplify this further.

1344

I think you'll need to return &PN if it was updated:

if (foldPHIArgIntToPtrToPHI(PN))
  return &PN;
Krishnakariya marked 2 inline comments as done.
Krishnakariya added inline comments.Aug 3 2021, 3:26 AM
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
307

Thanks, I have updated the patch.