[GVNSink] Fix incorrect codegen with respect to GEPs #85333

[hiraditya]

As mentioned in #68882 and https://discourse.llvm.org/t/rfc-replacing-getelementptr-with-ptradd/68699

Gep arithmetic isn't consistent with different types. GVNSink didn't realize this and sank all geps
as long as their operands can be wired via PHIs
in a post-dominator.

Fixes: #85333

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: AdityaK (hiraditya)

Changes

As mentioned in #68882 and https://discourse.llvm.org/t/rfc-replacing-getelementptr-with-ptradd/68699

Gep arithmetic isn't consistent with different types. GVNSink didn't realize this and sank all geps
as long as their operands can be wired via PHIs
in a post-dominator.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/GVNSink.cpp (+19-8)
• (added) llvm/test/Transforms/GVNSink/different-gep-types.ll (+47)

diff --git a/llvm/lib/Transforms/Scalar/GVNSink.cpp b/llvm/lib/Transforms/Scalar/GVNSink.cpp
index d4907326eb0a5a..a64ae46d2a72a3 100644
--- a/llvm/lib/Transforms/Scalar/GVNSink.cpp
+++ b/llvm/lib/Transforms/Scalar/GVNSink.cpp
@@ -719,21 +719,32 @@ GVNSink::analyzeInstructionForSinking(LockstepReverseIterator &LRI,
   // try and continue making progress.
   Instruction *I0 = NewInsts[0];
 
-  // If all instructions that are going to participate don't have the same
-  // number of operands, we can't do any useful PHI analysis for all operands.
-  auto hasDifferentNumOperands = [&I0](Instruction *I) {
-    return I->getNumOperands() != I0->getNumOperands();
+  auto hasDifferentOperands = [&I0](Instruction *I) {
+    // If all instructions that are going to participate don't have the same
+    // number of operands, we can't do any useful PHI analysis for all operands.
+    if (I->getNumOperands() != I0->getNumOperands())
+      return true;
+    // Having different source element types may result in incorrect
+    // pointer arithmetic on geps(github.com/llvm/llvm-project/issues/85333)
+    if (auto *II = dyn_cast<GetElementPtrInst>(I)) {
+      auto I0I = cast<GetElementPtrInst>(I0);
+      return II->getSourceElementType() != I0I->getSourceElementType();
+    }
+    return false;
   };
-  if (any_of(NewInsts, hasDifferentNumOperands))
+
+  if (any_of(NewInsts, hasDifferentOperands))
     return std::nullopt;
 
   for (unsigned OpNum = 0, E = I0->getNumOperands(); OpNum != E; ++OpNum) {
     ModelledPHI PHI(NewInsts, OpNum, ActivePreds);
     if (PHI.areAllIncomingValuesSame())
       continue;
-    if (!canReplaceOperandWithVariable(I0, OpNum))
-      // We can 't create a PHI from this instruction!
-      return std::nullopt;
+    for (auto &Candidate : NewInsts) {
+      if (!canReplaceOperandWithVariable(Candidate, OpNum))
+        // We can 't create a PHI from this instruction!
+        return std::nullopt;
+    }
     if (NeededPHIs.count(PHI))
       continue;
     if (!PHI.areAllIncomingValuesSameType())
diff --git a/llvm/test/Transforms/GVNSink/different-gep-types.ll b/llvm/test/Transforms/GVNSink/different-gep-types.ll
new file mode 100644
index 00000000000000..5016be997a2775
--- /dev/null
+++ b/llvm/test/Transforms/GVNSink/different-gep-types.ll
@@ -0,0 +1,47 @@
+; RUN: opt -passes=gvn-sink -S %s | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "thumbv7em-none-unknown-eabi"
+
+%"struct.std::pair" = type <{ i32, %struct.substruct, [2 x i8] }>
+%struct.substruct = type { i8, i8 }
+%"struct.std::random_access_iterator_tag" = type { i8 }
+
+; CHECK: if.end6
+; CHECK: %incdec.ptr.sink = phi ptr [ %incdec.ptr, %if.then ], [ %incdec.ptr4, %if.then3 ], [ %add.ptr, %if.else5 ]
+; CHECK-NEXT: store ptr %incdec.ptr.sink, ptr %__i, align 4
+
+define linkonce_odr dso_local void @__advance(ptr noundef nonnull align 4 dereferenceable(4) %__i, i32 noundef %__n) local_unnamed_addr {
+entry:
+  %0 = call i1 @llvm.is.constant.i32(i32 %__n)
+  %cmp = icmp eq i32 %__n, 1
+  %or.cond = and i1 %0, %cmp
+  br i1 %or.cond, label %if.then, label %if.else
+
+if.then:                                          ; preds = %entry
+  %1 = load ptr, ptr %__i, align 4
+  %incdec.ptr = getelementptr inbounds i8, ptr %1, i32 8
+  store ptr %incdec.ptr, ptr %__i, align 4
+  br label %if.end6
+
+if.else:                                          ; preds = %entry
+  %2 = call i1 @llvm.is.constant.i32(i32 %__n)
+  %cmp2 = icmp eq i32 %__n, -1
+  %or.cond7 = and i1 %2, %cmp2
+  br i1 %or.cond7, label %if.then3, label %if.else5
+
+if.then3:                                         ; preds = %if.else
+  %3 = load ptr, ptr %__i, align 4
+  %incdec.ptr4 = getelementptr inbounds i8, ptr %3, i32 -8
+  store ptr %incdec.ptr4, ptr %__i, align 4
+  br label %if.end6
+
+if.else5:                                         ; preds = %if.else
+  %4 = load ptr, ptr %__i, align 4
+  %add.ptr = getelementptr inbounds %"struct.std::pair", ptr %4, i32 %__n
+  store ptr %add.ptr, ptr %__i, align 4
+  br label %if.end6
+
+if.end6:                                          ; preds = %if.then3, %if.else5, %if.then
+  ret void
+}

[ilovepi]

Overall, this seems fine to me, but I do wonder about the situation arising in the first place. I was under the impression that GEPs had been canonicalized to always use i8 now when the IR is read in (e.g. auto upgrade). Is that logic in one of the simplification passes? Does that imply that such a pass should always run before GVNSink? IIRC, what I'm referring to is related to the ptradd proposal.

[nikic]

This is probably not the right way to fix this: GEPs are not the only instruction type that has non-operand state. Either there is some other place already taking this into account in GVNSink that needs to be adapted for GEPs, or GVNSink should be calling hasSameSpecialState() (or one of that family of functions).

[nikic]

Overall, this seems fine to me, but I do wonder about the situation arising in the first place. I was under the impression that GEPs had been canonicalized to always use i8 now when the IR is read in (e.g. auto upgrade). Is that logic in one of the simplification passes? Does that imply that such a pass should always run before GVNSink? IIRC, what I'm referring to is related to the ptradd proposal.

GVNSink still has to be correct when considered as a standalone pass. Besides, the i8 canonicalization is currently only applied to constant offsets, while GVNSink presumably also operates on non-constants.

[nikic]

As a warning: GVNSink is non-deterministic, which makes it easy for tests written in one environment to fail on some buildbots. I don't know whether it will affect this specific test. This non-determinism (and the fact that it prevents us from writing tests) has been blocking various fixes in this incredibly broken pass.

[ilovepi]

@nikic I filed #77852 a while back when we ran into this, but are there other known issues of non-determinism that you can point me to? This sounds like something worth fixing, since we've found GVNHoist/GVNSink save a decent amount of size for embedded targets, and it would be ideal if we could stabilize it enough to enable in the -Oz pipeline by default.

[ilovepi]

GVNSink still has to be correct when considered as a standalone pass.

Agreed. That was kind of what I was getting at, though I didn't express it that clearly. I guess my sentiment is "hey, if GVNSink needs the IR to be in a certain state, shouldn't it ensure it's in that state itself (e.g., by running some canonicalization routine)?".

Besides, the i8 canonicalization is currently only applied to constant offsets, while GVNSink presumably also operates on non-constants.

Ah, that is true, I had forgotten we only do that for constant offsets.

[hiraditya]

This is probably not the right way to fix this: GEPs are not the only instruction type that has non-operand state. Either there is some other place already taking this into account in GVNSink that needs to be adapted for GEPs, or GVNSink should be calling hasSameSpecialState() (or one of that family of functions).

i see. this makes it simpler. Thanks!

[nikic]

LGTM

[dyung]

@hiraditya, the test you added seems to be failing on windows bots:

• https://lab.llvm.org/buildbot/#/builders/216/builds/38357
• https://lab.llvm.org/buildbot/#/builders/233/builds/9396
• https://lab.llvm.org/buildbot/#/builders/235/builds/7335

Can you take a look and revert if you need time to investigate?

[hiraditya]

let me revert it first.

[nikic]

This is exactly the test non-determinism issue I've mentioned before. I think at this point we cannot make any fixes to GVNSink until the non-determinism issue is fixed and we can actually write tests for this pass.

[hiraditya]

makes sense. i'll update my investigation in the issue (#77852) created by @ilovepi