This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold Int2Ptr/PtrToInt if the ptr is dereferenceable
Needs ReviewPublic

Authored by Krishnakariya on Aug 10 2021, 6:13 AM.

Details

Summary

In this patch, we remove the int2ptr [ptr2int (p)] if p is dereferenceable.
If the user of the round-trip cast is dominated by a load/store instruction of the same address, then we
can replace round-trip cast with bitcast.

Diff Detail

Unit TestsFailed

Event Timeline

Krishnakariya created this revision.Aug 10 2021, 6:13 AM
Krishnakariya requested review of this revision.Aug 10 2021, 6:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2021, 6:13 AM
nick added a subscriber: nick.Aug 10 2021, 6:53 AM
Krishnakariya edited the summary of this revision. (Show Details)

Modified code and Added new tests

Hi,

I became to wonder what happens if a pointer is freed in the function.
For example:

// assume that %p is dereferenceable(8)
call void @g(%p);
%q = inttoptr (ptrtoint %p);
use(%q); // can we replace this with %p?

I guess the answer is no, because if @g freed %p and allocated an object at %p's location, replacing inttoptr(ptrtoint %p) with %p makes the program more undefined.

Is the semantics of dereferenceable(8) %p implying %p must not be freed in the function as well?
If it isn't I think we should check whether nofree flag is attached to the functions (@test, @test1, ...) as well.

Hi,

I became to wonder what happens if a pointer is freed in the function.
For example:

// assume that %p is dereferenceable(8)
call void @g(%p);
%q = inttoptr (ptrtoint %p);
use(%q); // can we replace this with %p?

I guess the answer is no, because if @g freed %p and allocated an object at %p's location, replacing inttoptr(ptrtoint %p) with %p makes the program more undefined.

Is the semantics of dereferenceable(8) %p implying %p must not be freed in the function as well?
If it isn't I think we should check whether nofree flag is attached to the functions (@test, @test1, ...) as well.

I agree with you, but this is a bug in isDereferenceableAndAlignedPointer. This API should receive the instruction where we want to prove the ptr is dereferenceable and take care of free, etc.
So I would not block this patch on this.

aqjune added inline comments.Aug 19 2021, 9:16 PM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
348

Then, what do you think about adding a nofree check at this point? @nlopes

Let's check whether CI's function has a nofree attribute. The checking will be fairly simple.

define void @test(i8* %X, i8* %Y, i8* %Z) *nofree* {
...
}

The code here looks very weird.
Please can you at least split this up into a preparatory NFC changes, the deref-based fold, and the domination-based fold?
Also, please precommit the tests, even if just locally for you, so that the patches show how the check lines change.

nlopes added inline comments.Aug 20 2021, 8:17 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
348

Well, nofree is still not very common. Which would render this optimization useless. Plus it would be unnecessary (and restrictive) to check once isDereferenceableAndAlignedPointer is fixed.
Maybe what we need is a bug report for isDereferenceableAndAlignedPointer as it seems Philip abandoned the work on nofree.

aqjune added inline comments.Aug 20 2021, 7:35 PM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
348

Okay, then I'm fine with the code leaving as it is here.
Could you split this patch into three as suggested by Lebedev? @Krishnakariya

Krishnakariya marked an inline comment as done.

Updated diff

The code here looks very weird.
Please can you at least split this up into a preparatory NFC changes, the deref-based fold, and the domination-based fold?
Also, please precommit the tests, even if just locally for you, so that the patches show how the check lines change.

Splited this patch into 3 parts: D108504 (NFC), D108506 (deref-based fold), and this one (dominator-based fold). Added precommit tests also.

lebedev.ri resigned from this revision.Jan 12 2023, 5:23 PM

This review seems to be stuck/dead, consider abandoning if no longer relevant.

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 5:23 PM
Herald added a subscriber: StephenFan. · View Herald Transcript