[PATCH] D117201: [ConstantHoist] Remove check for notional overindexing

Nikita Popov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 17 07:45:35 PST 2022


nikic updated this revision to Diff 400554.
nikic added a comment.

Okay, I see what you mean now. I've updated the implementation to perform an explicit inbounds check.

As the code requires a GlobalVariable base pointer, the notional overindexing check did effectively guarantee that it is inbounds, because there would be a bitcast sitting in between otherwise -- of course, this is not true anymore with opaque pointers, as the GEP type is entirely independent of the global type in that case.


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

https://reviews.llvm.org/D117201

Files:
  llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
  llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep-overindexing.ll


Index: llvm/test/Transforms/ConstantHoisting/ARM/const-hoist-gep-overindexing.ll
===================================================================
--- /dev/null
+++ 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
+}
+
Index: llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
+++ llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
@@ -414,6 +414,14 @@
   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 @@
   // 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 @@
 
   // 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;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D117201.400554.patch
Type: text/x-patch
Size: 3924 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220117/b81ba852/attachment.bin>


More information about the llvm-commits mailing list