[llvm] Fix incorrect codegen with respect to GEPs #85333 (PR #88440)

[via llvm-commits]

https://github.com/hiraditya updated https://github.com/llvm/llvm-project/pull/88440

>From 9594c7f51bcb7986cb8e86a3ce6eee9efb9accd1 Mon Sep 17 00:00:00 2001
From: AdityaK <hiraditya at msn.com>
Date: Thu, 11 Apr 2024 14:04:45 -0700
Subject: [PATCH] Fix incorrect codegen with respect to GEPs #85333

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.
---
 llvm/lib/Transforms/Scalar/GVNSink.cpp        | 27 +++++++----
 .../Transforms/GVNSink/different-gep-types.ll | 47 +++++++++++++++++++
 2 files changed, 66 insertions(+), 8 deletions(-)
 create mode 100644 llvm/test/Transforms/GVNSink/different-gep-types.ll

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
+}