[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#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?



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1994
+            if (isa<PHINode>(PHIUser))
+              PHI2 = dyn_cast<PHINode>(PHIUser);
+            // Not a 8-bit Pointer. Need to check shift amt as power of 2?
----------------
just cast.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1997
+            else if ((PHIUser->getOpcode() == Instruction::AShr ||
+                      PHIUser->getOpcode() == Instruction::LShr) &&
+                     PHIUser->hasOneUse() &&
----------------
These need to be part of proof + tested.

Should probably also use match here but thats not particularly important.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:2002
+            else
+              return nullptr;
+
----------------
Should be split to a seperate function, otherwise early returns may interfere with other folds.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:2012
+              if (!(U->hasOneUse() &&
+                    ((match(U, m_Or(m_Specific(PHI2), m_Value())) &&
+                      match(U->user_back(),
----------------
What is this `Or` doing here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157977