[llvm] d56b0ad - [ConstantHoist] Remove check for notional overindexing

[Nikita Popov via llvm-commits]

Author: Nikita Popov
Date: 2022-01-19T11:32:10+01:00
New Revision: d56b0ad441a34ae6c353c823969397b451f053a6

URL: https://github.com/llvm/llvm-project/commit/d56b0ad441a34ae6c353c823969397b451f053a6
DIFF: https://github.com/llvm/llvm-project/commit/d56b0ad441a34ae6c353c823969397b451f053a6.diff

LOG: [ConstantHoist] Remove check for notional overindexing

ConstantHoist currently only hoists GEPs if there is no notional
overindexing. As this transform only hoists address arithmetic,
it shouldn't care about whether any overindexing occurs or not.

There is one caveat: If the hoisted base GEP is inbounds, and a
later non-inbounds GEP is rewritten in terms of it, the value
may be incorrectly poisoned. To avoid this, restrict the transform
to inbounds GEPs for now, as the notional overindexing check
effectively did that as well. The inbounds restriction could be
dropped by dropping inbounds from the base GEP expression.

Differential Revision: https://reviews.llvm.org/D117201

Added: 
    llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep-overindexing.ll

Modified: 
    llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp b/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
index 37a7053d778e0..25e8c3ef3b482 100644
--- a/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
@@ -414,6 +414,14 @@ void ConstantHoistingPass::collectConstantCandidates(
   IntegerType *PtrIntTy = DL->getIntPtrType(*Ctx, GVPtrTy->getAddressSpace());
   APInt Offset(DL->getTypeSizeInBits(PtrIntTy), /*val*/0, /*isSigned*/true);
   auto *GEPO = cast<GEPOperator>(ConstExpr);
+
+  // TODO: If we have a mix of inbounds and non-inbounds GEPs, then basing a
+  // non-inbounds GEP on an inbounds GEP is potentially incorrect. Restrict to
+  // inbounds GEP for now -- alternatively, we could drop inbounds from the
+  // constant expression,
+  if (!GEPO->isInBounds())
+    return;
+
   if (!GEPO->accumulateConstantOffset(*DL, Offset))
     return;
 
@@ -470,7 +478,7 @@ void ConstantHoistingPass::collectConstantCandidates(
   // Visit constant expressions that have constant integers.
   if (auto ConstExpr = dyn_cast<ConstantExpr>(Opnd)) {
     // Handle constant gep expressions.
-    if (ConstHoistGEP && ConstExpr->isGEPWithNoNotionalOverIndexing())
+    if (ConstHoistGEP && isa<GEPOperator>(ConstExpr))
       collectConstantCandidates(ConstCandMap, Inst, Idx, ConstExpr);
 
     // Only visit constant cast expressions.
@@ -810,7 +818,7 @@ void ConstantHoistingPass::emitBaseConstants(Instruction *Base,
 
   // Visit constant expression.
   if (auto ConstExpr = dyn_cast<ConstantExpr>(Opnd)) {
-    if (ConstExpr->isGEPWithNoNotionalOverIndexing()) {
+    if (isa<GEPOperator>(ConstExpr)) {
       // Operand is a ConstantGEP, replace it.
       updateOperand(ConstUser.Inst, ConstUser.OpndIdx, Mat);
       return;

diff  --git a/llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep-overindexing.ll b/llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep-overindexing.ll
new file mode 100644
index 0000000000000..f2817f51b69fd
--- /dev/null
+++ b/llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep-overindexing.ll
@@ -0,0 +1,45 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -consthoist -consthoist-gep -S -o - %s | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "thumbv6m-none--musleabi"
+
+%0 = type { [10 x i32], [10 x i16] }
+
+ at global = external local_unnamed_addr global %0, align 4
+
+define void @test_inbounds() {
+; CHECK-LABEL: @test_inbounds(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    [[CONST:%.*]] = bitcast i16* getelementptr inbounds ([[TMP0:%.*]], %0* @global, i32 0, i32 1, i32 0) to i16*
+; CHECK-NEXT:    store i16 undef, i16* [[CONST]], align 2
+; CHECK-NEXT:    [[BASE_BITCAST:%.*]] = bitcast i16* [[CONST]] to i8*
+; CHECK-NEXT:    [[MAT_GEP:%.*]] = getelementptr i8, i8* [[BASE_BITCAST]], i32 2
+; CHECK-NEXT:    [[MAT_BITCAST:%.*]] = bitcast i8* [[MAT_GEP]] to i16*
+; CHECK-NEXT:    store i16 undef, i16* [[MAT_BITCAST]], align 2
+; CHECK-NEXT:    [[BASE_BITCAST1:%.*]] = bitcast i16* [[CONST]] to i8*
+; CHECK-NEXT:    [[MAT_GEP2:%.*]] = getelementptr i8, i8* [[BASE_BITCAST1]], i32 20
+; CHECK-NEXT:    [[MAT_BITCAST3:%.*]] = bitcast i8* [[MAT_GEP2]] to i16*
+; CHECK-NEXT:    store i16 undef, i16* [[MAT_BITCAST3]], align 2
+; CHECK-NEXT:    ret void
+;
+bb:
+  store i16 undef, i16* getelementptr inbounds (%0, %0* @global, i32 0, i32 1, i32 0)
+  store i16 undef, i16* getelementptr inbounds (%0, %0* @global, i32 0, i32 1, i32 1)
+  store i16 undef, i16* getelementptr inbounds (%0, %0* @global, i32 0, i32 1, i32 10)
+  ret void
+}
+
+define dso_local void @test_non_inbounds() {
+; CHECK-LABEL: @test_non_inbounds(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    store i16 undef, i16* getelementptr inbounds ([[TMP0:%.*]], %0* @global, i32 0, i32 1, i32 11), align 2
+; CHECK-NEXT:    store i16 undef, i16* getelementptr ([[TMP0]], %0* @global, i32 0, i32 1, i32 12), align 2
+; CHECK-NEXT:    ret void
+;
+bb:
+  store i16 undef, i16* getelementptr inbounds (%0, %0* @global, i32 0, i32 1, i32 11)
+  store i16 undef, i16* getelementptr (%0, %0* @global, i32 0, i32 1, i32 12)
+  ret void
+}
+