[PATCH] D138637: [InstCombine] Combine opaque pointer single index GEP and with src GEP which has result of array type

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2084
+  // GEP having single index.
+  if (PtrTy->isOpaquePointerTy() && Src->getResultElementType()->isArrayTy() &&
+      Src->getResultElementType() != GEP.getSourceElementType() &&
----------------
The `Src->getResultElementType()->isArrayTy()` check here is overly strict. Consider that the result type is something like `{ [0 x float] }`. This can be handled the same way as `[0 x float]` with an extra zero index (which you do already handle, if this check is dropped).

> Src gep can have multiple users which can be geps. This patch tries to optimize them aswell. See gepofgep4 test added below.

Hm, it looks like the existing transform does allow GEPs with multiple uses here, but this seems like a bug to me, because this means that we will end up repeating non-trivial address calculations, rather than performing them only once.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2112
+      NewIndices.clear();
+      NewIndices.append(Src->idx_begin(), Src->idx_end());
+      NewIndices.append(GEP.idx_begin(), GEP.idx_end());
----------------
Don't we need to only drop the last zero from NewIndices? Consider something like `getelementptr [3 x [2 x [1 x float]]], ptr %p, i64 %x` followed by `getelementptr float, ptr %gep, i64 %y`. I think your current code will convert this into `getelementptr [3 x [2 x [1 x float]]], ptr %p, i64 %x, i64 %y`, while the correct result would be `getelementptr [3 x [2 x [1 x float]]], ptr %p, i64 %x, i64 0, i64 %y`.


================
Comment at: llvm/test/Transforms/InstCombine/opaque-ptr-gep-of-gep.ll:1
+; RUN: opt -S -passes=instcombine -mtriple=x86_64-unknown-unknown %s | FileCheck %s
+
----------------
The triple can be dropped here. Please also use update_test_checks.py to generate check lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138637