This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold IntToPtr/PtrToInt to bitcast
ClosedPublic

Authored by Krishnakariya on Jun 29 2021, 1:28 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. This patch is the first one on this line.

Consider the example,

%i = ptrtoint i8* %X to i64
%p = inttoptr i64 %i to i16*   
%cmp = icmp eq i8* %load, %p

In this specific case, the inttoptr/ptrtoint optimization is correct as it only compares the pointers. In this patch, we fold inttoptr/ptrtoint to a bitcast (if src and dest types are different).

Diff Detail

Event Timeline

Krishnakariya created this revision.Jun 29 2021, 1:28 AM
Krishnakariya requested review of this revision.Jun 29 2021, 1:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2021, 1:28 AM

I think you need to check the type of the result of the ptrtoint. (instcombine will normally canonicalize to have the same width as the pointer type, but that isn't guaranteed to happen before this code runs.)

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4595

Might as well call CreateBitCast unconditionally; it has the same bailout internally.

Added the check and test case for the same.

efriedma added inline comments.Jul 6 2021, 12:08 PM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4593

I think you also need to check the size of p1? If the pointers are in different address spaces, the sizes might not match, so the bitcast will fail to verify. Again, maybe hard to trigger in practice because the ptrtoint destination type gets canonicalized.

4597

Minor nit: Value *NewOp0 = Builder.CreateBitCast(P, Op0Ty);

Krishnakariya marked an inline comment as done.

Updated the check.

efriedma accepted this revision.Jul 7 2021, 1:05 PM

LGTM

This revision is now accepted and ready to land.Jul 7 2021, 1:05 PM
nikic requested changes to this revision.Jul 7 2021, 1:12 PM

As this fold is currently subsumed by the more general inttoptr(ptrtoint(x)) fold, your test isn't really testing anything useful right now. Even without your added code, it folds to the same result.

I think you should add an option that disables the primary fold, and then use that option in the test. That way you can show that this particular case is still handled even once the general fold has been removed.

This revision now requires changes to proceed.Jul 7 2021, 1:12 PM
Krishnakariya marked 2 inline comments as done.

Added an command-line option disable-i2p-p2i-opt to opt to disable inttoptr/ptrtoint optimization.
I have removed the const keyword from the declaration of the CastResults array.
This change is temporary, and the array would be constified again.

nikic added inline comments.Jul 9 2021, 6:24 AM
llvm/lib/IR/Instructions.cpp
2798 ↗(On Diff #357492)

It would be better to modify case 7 below instead.

Krishnakariya marked an inline comment as done.
Krishnakariya added inline comments.Jul 9 2021, 6:40 AM
llvm/lib/IR/Instructions.cpp
2798 ↗(On Diff #357492)

Thanks, I've updated the patch.

nikic added a comment.Jul 10 2021, 1:30 AM

It would be good to pre-commit the tests, so the diff is clearly visible.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4588

The code in isEliminableCastPair() also checks that the address spaces match. Can you please add tests involving different address spaces?

Krishnakariya marked an inline comment as done.

Created a new function (simplifyIntToPtrRoundTripCast) which simplifies the inttoptr/ptrtoint cast.

xbolva00 added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4577

... and then this code can be simplified...

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
354

return nullptr instead Val? ...

Krishnakariya marked 2 inline comments as done.

simplified code for function "foldICmpWithCastOp".

xbolva00 added inline comments.Jul 15 2021, 3:26 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4577

if (Op0 || Op1)

4578

return new ICmpInst(ICmp.getPredicate(), Op0 ? Op0 : ICmp.getOperand(0), Op1 ? Op1 : ICmp.getOperand(1));

I think you should use NewOp0 or SimplifiedO0 or something like that.

Krishnakariya marked 2 inline comments as done.
aqjune added a subscriber: aqjune.Jul 15 2021, 6:51 PM
nikic accepted this revision.Jul 16 2021, 1:31 PM

LGTM

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
4574

Maybe add a brief comment here that explains why it is legal? Just "because icmp doesn't care about provenance" should do.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
352

You could directly cast to IntToPtrInst here.

This revision is now accepted and ready to land.Jul 16 2021, 1:31 PM
Krishnakariya marked 2 inline comments as done.

Simplified code.

This revision was landed with ongoing or failed builds.Jul 18 2021, 2:14 PM
This revision was automatically updated to reflect the committed changes.