[PATCH] D157977: [InstCombine] OptimizePointerDifference when a gep has phi ptr

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D157977#4643062 <https://reviews.llvm.org/D157977#4643062>, @bipmis wrote:

> In D157977#4641397 <https://reviews.llvm.org/D157977#4641397>, @goldstein.w.n wrote:
>
>> In D157977#4636857 <https://reviews.llvm.org/D157977#4636857>, @bipmis wrote:
>>
>>> Rebase patch after pre-committing tests. Address review comments.
>>> Alive 2 links
>>> https://alive2.llvm.org/ce/z/wZJfzE
>>> https://alive2.llvm.org/ce/z/izA8ij
>>
>> These can/should be simplified a great deal.
>> For example if I understand correctly youre trying to transform the following pattern:
>>
>>   define i1 @src(ptr %p, i64 %A, i64 %B, i64 %C, i1 %c) {
>>   entry:
>>     br i1 %c, label %true, label %false
>>   
>>   true:
>>     %p_gepA = getelementptr i8, ptr %p, i64 %A
>>     br label %false
>>   false:
>>     %p_phi = phi ptr [ %p_gepA, %true ], [ %p, %entry ]
>>     %phi_gepC = getelementptr i8, ptr %p_phi, i64 %C
>>   
>>     %gepC_int = ptrtoint ptr %phi_gepC to i64
>>     %p_gepB = getelementptr i8, ptr %p, i64 %B
>>     %gepB_int = ptrtoint ptr %p_gepB to i64
>>     %sub = sub i64 %gepC_int, %gepB_int
>>     %r = icmp eq i64 %sub, 0
>>     ret i1 %r
>>   }
>>
>> Also in general this patch appears to be doing a lot more than the summary indicates.
>> Please update to summary to be more precise.
>>
>> AFAICT Its really:
>>
>>   (phi, (sub (phi (phi GEPA), GEPB), GEPB), ...)
>>   or
>>   (phi, (ashr/lshr (sub (phi (phi GEPA), GEPB), GEPB), ...), ...)
>>
>> Is that correct?
>
> Not exactly. This transform will benefit but what I am trying to address is the test case which has a loop in the given pattern. If the sub which is the result of the loop results in an icmp, or(icmp) and based on the pattern it can be deduced that we can are better off with just the difference of index of the 2 pointers. In our example it then
> converts the sub to a sub(gep_index1, gep_index2). 
> It is also noted that other transforms in presence of TBAA can further reduce this reduction by eliminating the entire loop.
>
> As per my understanding it is a
> phi(sub(prttoint(A), ptrint(B)), ...)
> where A -> GEP(PHI(gep A, B)) and B is a ptr/GEP

So:

  (phi (sub (ptrtoint (gep (phi (gep A, (ptrtoint B)), ...)), (ptrtoint B), ...)

?

If so can you make the proofs / tests explicitly test that construct. As well if you are going to keep
the ashr/or stuff in also include that in the proofs/tests.

Even if the motivation is loops, the tests for the InstCombine codes shouldn't have to rely on that.
Its fine to keep some loop tests, but robust tests of all the cases in there simplest form (esp for the
proofs) makes it easier to review.

Also, can you split the tests to a seperate patch so we can see the diff generated by this patch?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157977/new/

https://reviews.llvm.org/D157977