[InstCombine] Lower multi-dimensional GEP to ptradd

[usha1830]

This change will help canonicalize multi-dimensional array geps with exactly one variable index and all other indices constant into a flat, single-index gep over the element type. This would cover cases such as:

%gep = getelementptr [9 x [9 x [9 x i32]]], ptr @arr, i64 0, i64 %i, i64 2, i64 3

https://alive2.llvm.org/ce/z/SWRKZd

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Usha Gupta (usha1830)

Changes

This change will help canonicalize multi-dimensional array geps with exactly one variable index and all other indices constant into a flat, single-index gep over the element type. This would cover cases such as:

%gep = getelementptr [9 x [9 x [9 x i32]]], ptr @<!-- -->arr, i64 0, i64 %i, i64 2, i64 3

---

Full diff: https://github.com/llvm/llvm-project/pull/150383.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+9)

diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index e2a9255ca9c6e..9b148e523b7a7 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2995,6 +2995,15 @@ static bool shouldCanonicalizeGEPToPtrAdd(GetElementPtrInst &GEP) {
                                  m_Shl(m_Value(), m_ConstantInt())))))
     return true;
 
+  // Flatten multidimensional GEPs with one variable index.
+  unsigned NumVarIndices = 0;
+  for (unsigned i = 1; i < GEP.getNumOperands(); ++i) {
+    if (!isa<ConstantInt>(GEP.getOperand(i)))
+      ++NumVarIndices;
+  }
+  if (NumVarIndices == 1)
+    return true;
+
   // gep (gep %p, C1), %x, C2 is expanded so the two constants can
   // possibly be merged together.
   auto PtrOpGep = dyn_cast<GEPOperator>(PtrOp);

[nikic]

I think this change is going to have a very big impact on other transforms and will require lots of mitigation before it's viable. Especially after #137297 this is going to convert all GEPs into ptradd representation.

[usha1830]

I think this change is going to have a very big impact on other transforms and will require lots of mitigation before it's viable. Especially after #137297 this is going to convert all GEPs into ptradd representation.

This patch helps in fixing the regression seen in this issue #143219.
And yes, I see a lot of test failures and am looking into addressing those.

[vzakhari]

Hi @usha1830! Thank you for working on this!

And yes, I see a lot of test failures and am looking into addressing those.

Please let me know if I can help investigating/resolving the failures. I am looking forward to enabling the noalias setting in Flang, so if there is a work to share, I am here to help.

[usha1830]

Please let me know if I can help investigating/resolving the failures. I am looking forward to enabling the noalias setting in Flang, so if there is a work to share, I am here to help.

@vzakhari After adding more constraints to the GEP transformation, all tests are now passing.

However, with the latest LLVM main, I’m seeing that this patch no longer gives the same performance benefit as before.

@nikic Do you think the GEP lowering I’m trying in this PR would still be generally useful for InstCombine or CodeGen? For example, I’m lowering:

%gep = getelementptr [10 x [10 x [10 x i32]]], ptr @base, i64 0, i64 %i, i64 2, i64 5

to 

%idx1 = mul i64 %i, 400
%tmp0 = getelementptr i8, ptr @base, i64 %idx1
%gep = getelementptr i8, ptr %tmp0, i64 100

Is there value in this sort of flattening/canonicalization after your latest changes around splitting/lowering geps in the current LLVM codebase?

Would appreciate your thoughts or a review. Thank you!

[nikic]

The ongoing work on splitting GEPs should convert this into:

%gep = getelementptr [10 x [10 x i32]], ptr @base, i64 %i
%gep2 = getelementptr i8, ptr %gep, i64 100

We're not yet ready to generally convert the first one to use gep + mul, because not all optimizations will be able to deal with it.

It's possible to carve out exceptions to do this ahead of time (we already do this sometimes), but from reading #143219 I don't have a good understanding of when, and more importantly, why this specific case is beneficial. It just seems to perturb things slightly in a way that happens to work out for this specific benchmark?

[usha1830]

Thank you for the quick response.

We were working for a specific benchmark. However, it is important to look at the general usefulness.

The ongoing work on splitting GEPs should convert this into:

%gep = getelementptr [10 x [10 x i32]], ptr @base, i64 %i
%gep2 = getelementptr i8, ptr %gep, i64 100

We're not yet ready to generally convert the first one to use gep + mul, because not all optimizations will be able to deal with it.

That sounds reasonable. I will close this PR as this is not adding much value. We can revisit this in future if required.