[InstCombine] Make foldCmpLoadFromIndexedGlobal more resiliant to non-array geps. #150639
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
llvm:instcombine
Member
|
@llvm/pr-subscribers-llvm-transforms Author: David Green (davemgreen) ChangesMy understanding is that gep [n x i8] and gep i8 can be treated equivalently - the array type conveys no extra information and could be removed. This goes through foldCmpLoadFromIndexedGlobal and tries to make it work for non-array gep types, so long as the index type still matches the array being loaded. Full diff: https://github.com/llvm/llvm-project/pull/150639.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index da9b12686a8f1..bd96edab201b5 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -113,10 +113,16 @@ Instruction *InstCombinerImpl::foldCmpLoadFromIndexedGlobal(
LoadInst *LI, GetElementPtrInst *GEP, GlobalVariable *GV, CmpInst &ICI,
ConstantInt *AndCst) {
if (LI->isVolatile() || LI->getType() != GEP->getResultElementType() ||
- GV->getValueType() != GEP->getSourceElementType() || !GV->isConstant() ||
+ !GV->getValueType()->isArrayTy() || !GV->isConstant() ||
!GV->hasDefinitiveInitializer())
return nullptr;
+ Type *GEPSrcEltTy = GEP->getSourceElementType();
+ if (GEPSrcEltTy->isArrayTy())
+ GEPSrcEltTy = GEPSrcEltTy->getArrayElementType();
+ if (GV->getValueType()->getArrayElementType() != GEPSrcEltTy)
+ return nullptr;
+
Constant *Init = GV->getInitializer();
if (!isa<ConstantArray>(Init) && !isa<ConstantDataArray>(Init))
return nullptr;
@@ -127,21 +133,27 @@ Instruction *InstCombinerImpl::foldCmpLoadFromIndexedGlobal(
return nullptr;
// There are many forms of this optimization we can handle, for now, just do
- // the simple index into a single-dimensional array.
+ // the simple index into a single-dimensional array or elements of equal size.
//
- // Require: GEP GV, 0, i {{, constant indices}}
- if (GEP->getNumOperands() < 3 || !isa<ConstantInt>(GEP->getOperand(1)) ||
- !cast<ConstantInt>(GEP->getOperand(1))->isZero() ||
- isa<Constant>(GEP->getOperand(2)))
+ // Require: GEP [n x i8] GV, 0, Idx {{, constant indices}}
+ // Or: GEP i8 GV, Idx
+
+ SmallVector<unsigned, 4> LaterIndices;
+ unsigned GEPIdxOp = 1;
+ if (GEP->getSourceElementType()->isArrayTy()) {
+ GEPIdxOp = 2;
+ if (!match(GEP->getOperand(1), m_ZeroInt()))
+ return nullptr;
+ }
+ if (GEP->getNumOperands() < GEPIdxOp + 1 ||
+ isa<Constant>(GEP->getOperand(GEPIdxOp)))
return nullptr;
// Check that indices after the variable are constants and in-range for the
// type they index. Collect the indices. This is typically for arrays of
// structs.
- SmallVector<unsigned, 4> LaterIndices;
-
Type *EltTy = Init->getType()->getArrayElementType();
- for (unsigned i = 3, e = GEP->getNumOperands(); i != e; ++i) {
+ for (unsigned i = GEPIdxOp + 1, e = GEP->getNumOperands(); i != e; ++i) {
ConstantInt *Idx = dyn_cast<ConstantInt>(GEP->getOperand(i));
if (!Idx)
return nullptr; // Variable index.
@@ -290,7 +302,7 @@ Instruction *InstCombinerImpl::foldCmpLoadFromIndexedGlobal(
// Now that we've scanned the entire array, emit our new comparison(s). We
// order the state machines in complexity of the generated code.
- Value *Idx = GEP->getOperand(2);
+ Value *Idx = GEP->getOperand(GEPIdxOp);
// If the index is larger than the pointer offset size of the target, truncate
// the index down like the GEP would do implicitly. We don't have to do this
diff --git a/llvm/test/Transforms/InstCombine/load-cmp.ll b/llvm/test/Transforms/InstCombine/load-cmp.ll
index 12be81b8f815d..485da6d2aaec4 100644
--- a/llvm/test/Transforms/InstCombine/load-cmp.ll
+++ b/llvm/test/Transforms/InstCombine/load-cmp.ll
@@ -68,7 +68,6 @@ define i1 @test1_noinbounds_as1(i32 %x) {
%q = load i16, ptr addrspace(1) %p
%r = icmp eq i16 %q, 0
ret i1 %r
-
}
define i1 @test1_noinbounds_as2(i64 %x) {
@@ -81,7 +80,17 @@ define i1 @test1_noinbounds_as2(i64 %x) {
%q = load i16, ptr addrspace(2) %p
%r = icmp eq i16 %q, 0
ret i1 %r
+}
+define i1 @test1_noarrayty(i32 %X) {
+; CHECK-LABEL: @test1_noarrayty(
+; CHECK-NEXT: [[R:%.*]] = icmp eq i32 [[X:%.*]], 9
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %P = getelementptr inbounds i16, ptr @G16, i32 %X
+ %Q = load i16, ptr %P
+ %R = icmp eq i16 %Q, 0
+ ret i1 %R
}
define i1 @test2(i32 %X) {
@@ -104,7 +113,17 @@ define i1 @test3(i32 %X) {
%Q = load double, ptr %P
%R = fcmp oeq double %Q, 1.0
ret i1 %R
+}
+define i1 @test3_noarrayty(i32 %X) {
+; CHECK-LABEL: @test3_noarrayty(
+; CHECK-NEXT: [[R:%.*]] = icmp eq i32 [[X:%.*]], 1
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %P = getelementptr inbounds double, ptr @GD, i32 %X
+ %Q = load double, ptr %P
+ %R = fcmp oeq double %Q, 1.0
+ ret i1 %R
}
define i1 @test4(i32 %X) {
@@ -333,6 +352,17 @@ define i1 @test10_struct_arr_noinbounds_i64(i64 %x) {
ret i1 %r
}
+define i1 @test10_struct_arr_noarrayty(i32 %x) {
+; CHECK-LABEL: @test10_struct_arr_noarrayty(
+; CHECK-NEXT: [[R:%.*]] = icmp ne i32 [[X:%.*]], 1
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %p = getelementptr inbounds %Foo, ptr @GStructArr, i32 %x, i32 2
+ %q = load i32, ptr %p
+ %r = icmp eq i32 %q, 9
+ ret i1 %r
+}
+
@table = internal constant [2 x ptr] [ptr @g, ptr getelementptr (i8, ptr @g, i64 4)], align 16
@g = external global [2 x i32]
|
This was referenced Jul 25, 2025
Member
|
IIRC #67093 is our final goal (i.e., cmp + load + gep i8/ptradd). But it looks stalled. |
dtcxzyw
approved these changes
Jul 26, 2025
Member
dtcxzyw
left a comment
dtcxzyw
left a comment
There was a problem hiding this comment.
LG. It looks like we are still far from the type-independent solution. I think this patch may be a simple mitigation. As for regressions caused by this patch, we may avoid emitting multiple icmps when the load has other users.
nikic
approved these changes
Jul 26, 2025
Contributor
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
I'm fine with this as well. This is not ideal, but still an improvement.
Contributor
There was a problem hiding this comment.
Why move the declaration of LaterIndices?
…-array geps. My understanding is that gep [n x i8] and gep i8 can be treated equivalently - the array type conveys no extra information and could be removed. This goes through foldCmpLoadFromIndexedGlobal and tries to make it work for non-array gep types, so long as the index type still matches the array being loaded.
davemgreen
force-pushed
the
gh-ic-gepcmpload
branch
from
f7edd3d to
a912a7c
Compare
Collaborator
Author
|
I should explain where this comes from, as there are a number of related issues going on at the same time. It was from a report I had of this code behaving differently between func1 and func2, just because of the different types of the GEP. One pulls an add out of the loop with rbit+ctz in it, the other doesn't. There are probably several ways to look at addressing that, but I figured the structs can be removed from the geps and that gets us closer to where we want to be. |
Member
Yeah, we plan to strip leading zero indices. Then we can see if it breaks some existing transforms that only work with array source element types. |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/13589 Here is the relevant piece of the build log for the reference |
Contributor
|
Hi @davemgreen The phi node is getting replaced with a branch that eventually leads to an infinite loop. The test run faults with "Invalid Instruction" on Neoverse-V2 platform. Can you please have a look and let me know if this can be fixed? |
Collaborator
Author
It looks like your code loads from an array that could be one of [97, 98, 99], then exits the loop if it is == 0. As that can never happen (without reading off the end of the array), it never exits the loops. I'm guessing the original had llvm.loop metadata that made infinite loops UB (it came from c++)? This sounds like standard C++ UB again if this is representative of the original. |
Contributor
Thanks, that makes sense. Yes, the code is indeed coming from C++. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
My understanding is that gep [n x i8] and gep i8 can be treated equivalently - the array type conveys no extra information and could be removed. This goes through foldCmpLoadFromIndexedGlobal and tries to make it work for non-array gep types, so long as the index type still matches the array being loaded.