From c5b3251dca632cd1b062555e75f26384b8243221 Mon Sep 17 00:00:00 2001 From: AdityaK <1894981+hiraditya@users.noreply.github.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. Fixes: #85333 --- llvm/lib/Transforms/Scalar/GVNSink.cpp | 9 +- .../Transforms/GVNSink/different-gep-types.ll | 101 ++++++++++++++++++ 2 files changed, 105 insertions(+), 5 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..f5f76b58a75326 100644 --- a/llvm/lib/Transforms/Scalar/GVNSink.cpp +++ b/llvm/lib/Transforms/Scalar/GVNSink.cpp @@ -719,12 +719,11 @@ 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) { + return !I0->isSameOperationAs(I); }; - if (any_of(NewInsts, hasDifferentNumOperands)) + + if (any_of(NewInsts, hasDifferentOperands)) return std::nullopt; for (unsigned OpNum = 0, E = I0->getNumOperands(); OpNum != E; ++OpNum) { 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..77cdc8a94f97c2 --- /dev/null +++ b/llvm/test/Transforms/GVNSink/different-gep-types.ll @@ -0,0 +1,101 @@ +; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4 +; 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" + +%"struct.std::pair" = type <{ i32, %struct.substruct, [2 x i8] }> +%struct.substruct = type { i8, i8 } +%"struct.std::random_access_iterator_tag" = type { i8 } + +; Check that gep is not sunk as they are of different types. +define void @bar(ptr noundef nonnull align 4 dereferenceable(4) %__i, i32 noundef %__n) { +; CHECK-LABEL: define void @bar( +; CHECK-SAME: ptr noundef nonnull align 4 dereferenceable(4) [[__I:%.*]], i32 noundef [[__N:%.*]]) { +; CHECK-NEXT: entry: +; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[__N]], 1 +; CHECK-NEXT: br i1 [[CMP]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]] +; CHECK: if.then: +; CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[__I]], align 4 +; CHECK-NEXT: [[INCDEC_PTR4:%.*]] = getelementptr inbounds i8, ptr [[TMP0]], i32 -8 +; CHECK-NEXT: br label [[IF_END6:%.*]] +; CHECK: if.else: +; CHECK-NEXT: [[TMP1:%.*]] = load ptr, ptr [[__I]], align 4 +; CHECK-NEXT: [[ADD_PTR:%.*]] = getelementptr inbounds %"struct.std::pair", ptr [[TMP1]], i32 [[__N]] +; CHECK-NEXT: br label [[IF_END6]] +; CHECK: if.end6: +; CHECK-NEXT: [[INCDEC_PTR_SINK:%.*]] = phi ptr [ [[INCDEC_PTR4]], [[IF_THEN]] ], [ [[ADD_PTR]], [[IF_ELSE]] ] +; CHECK-NEXT: store ptr [[INCDEC_PTR_SINK]], ptr [[__I]], align 4 +; CHECK-NEXT: ret void +; +entry: + %cmp = icmp eq i32 %__n, 1 + br i1 %cmp, label %if.then, label %if.else + +if.then: + %3 = load ptr, ptr %__i, align 4 + %incdec.ptr4 = getelementptr inbounds i8, ptr %3, i32 -8 + br label %if.end6 + +if.else: + %4 = load ptr, ptr %__i, align 4 + %add.ptr = getelementptr inbounds %"struct.std::pair", ptr %4, i32 %__n + br label %if.end6 + +if.end6: + %incdec.ptr.sink = phi ptr [ %incdec.ptr4, %if.then ], [ %add.ptr, %if.else ] + store ptr %incdec.ptr.sink, ptr %__i, align 4 + ret void +} + +; Check that load,gep, and store are all sunk as they are safe to do. +define void @foo(ptr noundef nonnull align 4 dereferenceable(4) %__i, i32 noundef %__n) { +; CHECK-LABEL: define void @foo( +; CHECK-SAME: ptr noundef nonnull align 4 dereferenceable(4) [[__I:%.*]], i32 noundef [[__N:%.*]]) { +; CHECK-NEXT: entry: +; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[__N]], 1 +; CHECK-NEXT: br i1 [[CMP]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]] +; CHECK: if.then: +; CHECK-NEXT: br label [[IF_END6:%.*]] +; CHECK: if.else: +; CHECK-NEXT: [[CMP2:%.*]] = icmp eq i32 [[__N]], -1 +; CHECK-NEXT: br i1 [[CMP2]], label [[IF_THEN3:%.*]], label [[IF_ELSE5:%.*]] +; CHECK: if.then3: +; CHECK-NEXT: br label [[IF_END6]] +; CHECK: if.else5: +; CHECK-NEXT: br label [[IF_END6]] +; CHECK: if.end6: +; CHECK-NEXT: [[DOTSINK1:%.*]] = phi i32 [ 8, [[IF_THEN]] ], [ -8, [[IF_THEN3]] ], [ -4, [[IF_ELSE5]] ] +; CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[__I]], align 4 +; CHECK-NEXT: [[INCDEC_PTR:%.*]] = getelementptr inbounds i8, ptr [[TMP0]], i32 [[DOTSINK1]] +; CHECK-NEXT: store ptr [[INCDEC_PTR]], ptr [[__I]], align 4 +; CHECK-NEXT: ret void +; +entry: + %cmp = icmp eq i32 %__n, 1 + br i1 %cmp, label %if.then, label %if.else + +if.then: + %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: + %cmp2 = icmp eq i32 %__n, -1 + br i1 %cmp2, label %if.then3, label %if.else5 + +if.then3: + %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: + %4 = load ptr, ptr %__i, align 4 + %add.ptr = getelementptr inbounds i8, ptr %4, i32 -4 + store ptr %add.ptr, ptr %__i, align 4 + br label %if.end6 + +if.end6: + ret void +}