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

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added reviewers: efriedma, zzheng.
Herald added subscribers: arphaman, hiraditya.
nikic requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

ConstantHoist currently only hoists GEPs if there is no notional overindexing. However, I don't see in what way overindexing is supposed to be relevant to this transform. I believe the only part it cares about is that the GEP has constant indices.

In the attached test case this makes ConstantHoist apply to the GEP with trailing `10` index, which is nominally out of bounds. (The corresponding store may not even be UB, though even if it were there is no reason why this transform should care.)


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,37 @@
+; 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 dso_local local_unnamed_addr global %0, align 4
+
+define dso_local void @zot() {
+; CHECK-LABEL: @zot(
+; 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 4
+; CHECK-NEXT:    [[MAT_BITCAST3:%.*]] = bitcast i8* [[MAT_GEP2]] to i16*
+; CHECK-NEXT:    store i16 undef, i16* [[MAT_BITCAST3]], align 2
+; CHECK-NEXT:    [[BASE_BITCAST4:%.*]] = bitcast i16* [[CONST]] to i8*
+; CHECK-NEXT:    [[MAT_GEP5:%.*]] = getelementptr i8, i8* [[BASE_BITCAST4]], i32 20
+; CHECK-NEXT:    [[MAT_BITCAST6:%.*]] = bitcast i8* [[MAT_GEP5]] to i16*
+; CHECK-NEXT:    store i16 undef, i16* [[MAT_BITCAST6]], 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 2)
+  store i16 undef, i16* getelementptr inbounds (%0, %0* @global, i32 0, i32 1, i32 10)
+  ret void
+}
+
Index: llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
+++ llvm/lib/Transforms/Scalar/ConstantHoisting.cpp
@@ -470,7 +470,8 @@
   // Visit constant expressions that have constant integers.
   if (auto ConstExpr = dyn_cast<ConstantExpr>(Opnd)) {
     // Handle constant gep expressions.
-    if (ConstHoistGEP && ConstExpr->isGEPWithNoNotionalOverIndexing())
+    auto *GEP = dyn_cast<GEPOperator>(ConstExpr);
+    if (ConstHoistGEP && GEP && GEP->hasAllConstantIndices())
       collectConstantCandidates(ConstCandMap, Inst, Idx, ConstExpr);
 
     // Only visit constant cast expressions.
@@ -810,7 +811,8 @@
 
   // Visit constant expression.
   if (auto ConstExpr = dyn_cast<ConstantExpr>(Opnd)) {
-    if (ConstExpr->isGEPWithNoNotionalOverIndexing()) {
+    auto *GEP = dyn_cast<GEPOperator>(ConstExpr);
+    if (GEP && GEP->hasAllConstantIndices()) {
       // Operand is a ConstantGEP, replace it.
       updateOperand(ConstUser.Inst, ConstUser.OpndIdx, Mat);
       return;


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