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

[Krishnakariya]

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.

[Krishnakariya]

Modified code and Added new tests

[Krishnakariya]

updated

[Krishnakariya]

updated.

[Krishnakariya]

ping

[aqjune]

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.

[nlopes]

In D107822#2953955, @aqjune wrote:

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]

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* {
...
}

[lebedev.ri]

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]

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]

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]

Updated diff

[Krishnakariya]

In D107822#2956860, @lebedev.ri wrote:

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]

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