[PATCH] D125845: [InstCombine] Canonicalize GEP of GEP by swapping constant-indexed GEP to the back

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I don't know enough to visualize the GEP corner-cases or the interaction with LICM, but I commented on general improvements to the patch.

Have you run any benchmarks to confirm there are improvements (no regressions)?



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1960-1964
+  // LICM takes priority over canonicalization swapping for merging constant-
+  // indexed GEP, because mergable constant-indexed GEPs can still be merged
+  // after they are hoisted out of the loop, but performing canonicalization
+  // first may miss the LICM opportunity.
+  bool shouldCanonicalizeSwap = true;
----------------
This code structure and the comments are confusing. I think it would be better to rearrange this like:
  bool LoopInvariantGEP = false;
  bool LoopInvariantSrc = false;
  if (LI && ...) {
    LoopInvariantGEP = L->isLoopInvariant(GEP.getOperand(1));
    LoopInvariantSrc = L->isLoopInvariant(Src->getOperand(1));
  }
  if (LoopInvariantGEP && !LoopInvariantSrc) {
    // do the existing transform
  }
  if (!(!LoopInvariantGEP && LoopInvariantSrc) && ...) {
    // do the new transform
  }

I think that matches the logic in this patch, but don't we need a set of tests with loops to make sure that does what you expect? I don't see any tests with loops.


================
Comment at: llvm/test/Transforms/InstCombine/gep-canonicalize-constant-indices.ll:33
 ;
   %1 = getelementptr inbounds [4 x i32], ptr %p, i64 %a, i64 1
   %2 = getelementptr inbounds i32, ptr %p, i64 %b
----------------
I don't know what this test and the next one are supposed to show. %1 is dead code, so it gets eliminated before anything else might have happened.


================
Comment at: llvm/test/Transforms/InstCombine/gep-canonicalize-constant-indices.ll:49
 
 ; Constant-indexed GEP are merged after swawpping.
 ; result = ((i32*) p + a) + 3
----------------
typo: swapping


================
Comment at: llvm/test/Transforms/InstCombine/gep-canonicalize-constant-indices.ll:98
 
 ; It is valid to swap if the second GEP has multiple uses.
 define ptr @multipleUses2(ptr %p, i64 %a) {
----------------
This test doesn't add much. AFAIK every transform in instcombine does RAUW for the final (root) value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125845