[InstCombine] Support gep nuw in icmp folds #118472
Conversation
Unsigned icmp of gep nuw folds to unsigned icmp of offsets. Unsigned icmp of gep nusw nuw folds to unsigned samesign icmp of offsets. Proofs: https://alive2.llvm.org/ce/z/VEwQY8
|
@llvm/pr-subscribers-llvm-transforms Author: Nikita Popov (nikic) ChangesUnsigned icmp of gep nuw folds to unsigned icmp of offsets. Unsigned icmp of gep nusw nuw folds to unsigned samesign icmp of offsets. Proofs: https://alive2.llvm.org/ce/z/VEwQY8 Full diff: https://github.com/llvm/llvm-project/pull/118472.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 579214c28fc304..ffc0b33171b8fc 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -690,13 +690,32 @@ Instruction *InstCombinerImpl::foldGEPICmp(GEPOperator *GEPLHS, Value *RHS,
if (!isa<GetElementPtrInst>(RHS))
RHS = RHS->stripPointerCasts();
+ auto CanFold = [Cond](GEPNoWrapFlags NW) {
+ if (ICmpInst::isEquality(Cond))
+ return true;
+
+ // Unsigned predicates can be folded if the GEPs have *any* nowrap flags.
+ assert(ICmpInst::isUnsigned(Cond));
+ return NW != GEPNoWrapFlags::none();
+ };
+
+ auto NewICmp = [Cond](GEPNoWrapFlags NW, Value *Op1, Value *Op2) {
+ if (!NW.hasNoUnsignedWrap()) {
+ // Convert signed to unsigned comparison.
+ return new ICmpInst(ICmpInst::getSignedPredicate(Cond), Op1, Op2);
+ }
+
+ auto *I = new ICmpInst(Cond, Op1, Op2);
+ I->setSameSign(NW.hasNoUnsignedSignedWrap());
+ return I;
+ };
+
Value *PtrBase = GEPLHS->getOperand(0);
- if (PtrBase == RHS &&
- (GEPLHS->hasNoUnsignedSignedWrap() || ICmpInst::isEquality(Cond))) {
+ if (PtrBase == RHS && CanFold(GEPLHS->getNoWrapFlags())) {
// ((gep Ptr, OFFSET) cmp Ptr) ---> (OFFSET cmp 0).
Value *Offset = EmitGEPOffset(GEPLHS);
- return new ICmpInst(ICmpInst::getSignedPredicate(Cond), Offset,
- Constant::getNullValue(Offset->getType()));
+ return NewICmp(GEPLHS->getNoWrapFlags(), Offset,
+ Constant::getNullValue(Offset->getType()));
}
if (GEPLHS->isInBounds() && ICmpInst::isEquality(Cond) &&
@@ -814,19 +833,18 @@ Instruction *InstCombinerImpl::foldGEPICmp(GEPOperator *GEPLHS, Value *RHS,
return replaceInstUsesWith(I, // No comparison is needed here.
ConstantInt::get(I.getType(), ICmpInst::isTrueWhenEqual(Cond)));
- else if (NumDifferences == 1 && NW.hasNoUnsignedSignedWrap()) {
+ else if (NumDifferences == 1 && CanFold(NW)) {
Value *LHSV = GEPLHS->getOperand(DiffOperand);
Value *RHSV = GEPRHS->getOperand(DiffOperand);
- // Make sure we do a signed comparison here.
- return new ICmpInst(ICmpInst::getSignedPredicate(Cond), LHSV, RHSV);
+ return NewICmp(NW, LHSV, RHSV);
}
}
- if (NW.hasNoUnsignedSignedWrap() || CmpInst::isEquality(Cond)) {
+ if (CanFold(NW)) {
// ((gep Ptr, OFFSET1) cmp (gep Ptr, OFFSET2) ---> (OFFSET1 cmp OFFSET2)
Value *L = EmitGEPOffset(GEPLHS, /*RewriteGEP=*/true);
Value *R = EmitGEPOffset(GEPRHS, /*RewriteGEP=*/true);
- return new ICmpInst(ICmpInst::getSignedPredicate(Cond), L, R);
+ return NewICmp(NW, L, R);
}
}
diff --git a/llvm/test/Transforms/InstCombine/icmp-gep.ll b/llvm/test/Transforms/InstCombine/icmp-gep.ll
index 1545d034b2ac31..1bc000cd6ebf1e 100644
--- a/llvm/test/Transforms/InstCombine/icmp-gep.ll
+++ b/llvm/test/Transforms/InstCombine/icmp-gep.ll
@@ -143,6 +143,44 @@ define i1 @ult_base_nusw(ptr %x, i64 %y) {
ret i1 %r
}
+define i1 @ugt_base_nuw(ptr %x, i64 %y) {
+; CHECK-LABEL: @ugt_base_nuw(
+; CHECK-NEXT: [[R:%.*]] = icmp ne i64 [[Y:%.*]], 0
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %g = getelementptr nuw i8, ptr %x, i64 %y
+ %r = icmp ugt ptr %g, %x
+ ret i1 %r
+}
+
+define i1 @ugt_base_nusw_nuw(ptr %x, i64 %y) {
+; CHECK-LABEL: @ugt_base_nusw_nuw(
+; CHECK-NEXT: [[R:%.*]] = icmp ne i64 [[Y:%.*]], 0
+; CHECK-NEXT: ret i1 [[R]]
+;
+ %g = getelementptr nusw nuw i8, ptr %x, i64 %y
+ %r = icmp ugt ptr %g, %x
+ ret i1 %r
+}
+
+define i1 @uge_base_nuw(ptr %x, i64 %y) {
+; CHECK-LABEL: @uge_base_nuw(
+; CHECK-NEXT: ret i1 true
+;
+ %g = getelementptr nuw i8, ptr %x, i64 %y
+ %r = icmp uge ptr %g, %x
+ ret i1 %r
+}
+
+define i1 @uge_base_nusw_nuw(ptr %x, i64 %y) {
+; CHECK-LABEL: @uge_base_nusw_nuw(
+; CHECK-NEXT: ret i1 true
+;
+ %g = getelementptr nusw nuw i8, ptr %x, i64 %y
+ %r = icmp uge ptr %g, %x
+ ret i1 %r
+}
+
define i1 @ugt_base_inbounds_commute(i64 %y) {
; CHECK-LABEL: @ugt_base_inbounds_commute(
; CHECK-NEXT: [[X:%.*]] = call ptr @getptr()
@@ -319,6 +357,43 @@ define i1 @test60_nusw_inbounds(ptr %foo, i64 %i, i64 %j) {
ret i1 %cmp
}
+define i1 @test60_nuw(ptr %foo, i64 %i, i64 %j) {
+; CHECK-LABEL: @test60_nuw(
+; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nuw i64 [[I:%.*]], 2
+; CHECK-NEXT: [[CMP:%.*]] = icmp ult i64 [[GEP1_IDX]], [[J:%.*]]
+; CHECK-NEXT: ret i1 [[CMP]]
+;
+ %gep1 = getelementptr nuw i32, ptr %foo, i64 %i
+ %gep2 = getelementptr nuw i8, ptr %foo, i64 %j
+ %cmp = icmp ult ptr %gep1, %gep2
+ ret i1 %cmp
+}
+
+define i1 @test60_nusw_nuw(ptr %foo, i64 %i, i64 %j) {
+; CHECK-LABEL: @test60_nusw_nuw(
+; CHECK-NEXT: [[GEP1_IDX:%.*]] = shl nuw nsw i64 [[I:%.*]], 2
+; CHECK-NEXT: [[CMP:%.*]] = icmp samesign ult i64 [[GEP1_IDX]], [[J:%.*]]
+; CHECK-NEXT: ret i1 [[CMP]]
+;
+ %gep1 = getelementptr nusw nuw i32, ptr %foo, i64 %i
+ %gep2 = getelementptr nusw nuw i8, ptr %foo, i64 %j
+ %cmp = icmp ult ptr %gep1, %gep2
+ ret i1 %cmp
+}
+
+define i1 @test60_nusw_nuw_mix(ptr %foo, i64 %i, i64 %j) {
+; CHECK-LABEL: @test60_nusw_nuw_mix(
+; CHECK-NEXT: [[GEP1:%.*]] = getelementptr nuw i32, ptr [[FOO:%.*]], i64 [[I:%.*]]
+; CHECK-NEXT: [[GEP2:%.*]] = getelementptr nusw i8, ptr [[FOO]], i64 [[J:%.*]]
+; CHECK-NEXT: [[CMP:%.*]] = icmp ult ptr [[GEP1]], [[GEP2]]
+; CHECK-NEXT: ret i1 [[CMP]]
+;
+ %gep1 = getelementptr nuw i32, ptr %foo, i64 %i
+ %gep2 = getelementptr nusw i8, ptr %foo, i64 %j
+ %cmp = icmp ult ptr %gep1, %gep2
+ ret i1 %cmp
+}
+
define i1 @test_gep_ult_no_inbounds(ptr %foo, i64 %i, i64 %j) {
; CHECK-LABEL: @test_gep_ult_no_inbounds(
; CHECK-NEXT: [[GEP1:%.*]] = getelementptr i32, ptr [[FOO:%.*]], i64 [[I:%.*]]
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/9514 Here is the relevant piece of the build log for the reference |
|
Also this one https://lab.llvm.org/buildbot/#/builders/66/builds/7009 |
This reverts commit f335364.
Reverts #118472 Breaks profile tests on i386 https://lab.llvm.org/buildbot/#/builders/66/builds/7009
|
@nikic Please confirm that revert is noticed. |
These bounds checks work on the result of the pointer addition -- but the pointer addition already asserts that no overflow may occur, so the checks are optimized away after llvm#118472. Avoid this by performing the addition in a way that permits overflow.
|
@vitalybuka I had some trouble reproducing this locally, but I think this change to the profiling runtime should fix the issue: #118782 |
These bounds checks work on the result of the pointer addition -- but the pointer addition already asserts that no overflow may occur, so the checks are optimized away after llvm#118472. Avoid this by performing the addition in a way that permits overflow.
These bounds checks work on the result of the pointer addition -- but the pointer addition already asserts that no overflow may occur, so the checks are optimized away after #118472. Avoid this by performing the addition in a way that permits overflow.
The profile runtime test failure this caused has been addressed in: #118782 ----- Unsigned icmp of gep nuw folds to unsigned icmp of offsets. Unsigned icmp of gep nusw nuw folds to unsigned samesign icmp of offsets. Proofs: https://alive2.llvm.org/ce/z/VEwQY8
|
@nikic Is it intended for this patch to optimize out the code in the Thanks in advance! using size_t = decltype(sizeof(int));
bool Check(const unsigned char* input, size_t size_input_in_bytes) {
return (input + size_input_in_bytes < input);
}
int main() {
unsigned char* buffer = new unsigned char[128];
return Check(buffer, ~static_cast<size_t>(0)) ? 0 : 1;
} |
|
@bgra8 Yes, optimizing out that kind of code is exactly the intention for this change (for bounds check elimination). |
|
I see @bgra8 already raised this, but maybe it's worth discussing further. There is a fair amount of test fallout from this (the profile merging issue in LLVM mentioned above, at least one issue in Chromium, and many more internally). We can fix those, but what concerns me are the issues not caught by tests. It doesn't seem unlikely that this optimization could open up security holes by removing these kinds of bounds checks. While such code was already technically broken, it would have been hard for developers to notice. Could -Wtautological-compare catch some of these (like the |
Yeah, I think this is a good idea.
UBSan should already flag this.
Not sure what exactly you have in mind here. |
Oh that's good. I guess we need to run it some more :-)
I was thinking that ASan could use the shadow memory to detect when one operand is pointing to bad memory which is more than one element past valid memory. That way we could perhaps catch (some) instances of Not sure if that's really practical though, and it still requires test coverage to catch anything. |
|
I filed #120214 for the warning. |
|
And #120222 for a possible implementation. |
This should probably directly operate on the It might make sense to experiment with this behind an option, as this probably adds a significant amount of overhead. (Though a separate check can be omitted for GEPs that are followed by an access, which is probably most of them.) |
We have found a couple of dozen of issues uncovered by this optimization, but I don't think in any of these cases ubsan was producing an error. So far the only reasonable way to investigate these problems was to selectively disable optimizations using Not having a good way to localize these problems is a serious concern for large codebases. |
See https://clang.godbolt.org/z/azr7rbv3c for a sample of ubsan reporting this. Are you running ubsan in the cases that actually overflow? It's not going to report something if there is no overflow... |
|
My bad, indeed, our cases are associated with invalid pointers rather than specifically with pointer overflow. But I suppose, a similar distribution is to be expected in other code bases, which means that there is no targeted sanitizer for this and diagnostic of issues resulting from this and related changes is non-trivial. Does performance gain justify this? |
|
Is this the only recent change that allows LLVM to make assumptions about pointer overflows? +1 to this being a problem. If we update clang in chromium, we pick this up and potentially introduce all kinds of silent security vulnerabilities. https://crbug.com/384391188 is an example where this erased a security pointer check. The check was technically invalid, but it worked prior to this, and after this it's an OOB write instead. And that's a case where we happened to have a test – several other places don't have a check probably. Another idea: C++ made signed integer overflow defined recently as far as I know, and unsigned integer overflow is also defined. Maybe it'd make sense to give clang a flag that says "give pointer overflow defined two's complement semantics"? Does Failing that, if this is the only patch, could we get a (possibly temporary) mllvm flag for turning this off? One reason why this is a bit of a problem: #116871 made -Wunused-private-field stricter, in a way that's not easy to put behind a new warning flag. It makes that warning fire a lot more. We cleaned up our codebase (required updating ~80 files) to be clean with the new warning, and we now have these options:
Neither alternative is particularly great. If there was a way to update but turn off this here for a while, that'd give us a better path forward. |
|
That's definitely not the only patch in this area. e21ab4d16b555c28ded307571d138f594f33e325 discovers more cases of pointer arithmetic-related UB. |
Can you share an example where you ran into an issue and ubsan did not diagnose? I would expect ubsan to reliably find all cases of this pattern (if there is test coverage).
The problem here is basically that that this is intended to optimize away redundant bounds checks inserted by programming languages that generally do bounds checks, or hardened library implementations. At this level, we cannot distinguish this from an incorrect hand-written bounds check in C/C++.
As far as I know signed integer overflow is still undefined in C++.
It's not the only patch, but I wouldn't mind adding an option for this one, as it's probably the highest impact one.
FWIW, I'd be happy to just revert that one entirely. I don't have a specific use case for that one, and am concerned that it currently slips through all the sanitizers. |
FWIW, most issues I mentioned in my previous comments are actually coming from e21ab4d, not this one. They happened to be tested together and I kind of conflated them in my head, hence the somewhat misleading report above. Given that we also discovered a bad interaction of that change with msan, reverting it would definitely help us. A test case is being worked on by a colleague, but I'm not quite sure how far they are with it. |
…#120460) Reverts #119225 due to the lack of sanitizer support, large potential of breaking code containing latent UB, non-trivial localization and investigation, and what seems to be a bad interaction with msan (a test is in the works). Related discussions: #119225 (comment) #118472 (comment)
I believe the sanitizer is Although, given that this blatant language defect has been acknowledged as a defect by the standards committee, perhaps it would make sense to either remove or recategorize that check in UBSan now. |
Yes, I agree that we should change this to align with the newly adopted C semantics, and the change being part of https://www.open-std.org/jtc1/sc22/wg14/www/previous.html should give us the leeway to do this for old standards versions (combined with the fact that we have never used this for optimization in the first place). This should just be a matter of always using the C++ logic in: llvm-project/clang/lib/CodeGen/CGExprScalar.cpp Line 5869 in 2b9abf0 |
Thanks for pointing this out! I confirmed that that's true. The documentation only talks about it affecting signed overflow. We'll switch to that, at least for now. It comes with a noticeable binary size hit, but it seems like the least bad option for now. (The reverts that landed don't seem to help some of the pointer checks that we saw getting deleted, and the compiler warning also didn't catch them due to the addition being stored in a variable before being compared.)
Yes, that'd be great. I think gcc has that flag already. If it doesn't yet, this should get a scary-looking release notes entry that mentions possible mitigations :) (Also, thank you for working on making llvm / clang better in removing unnecessary bounds checks! This is fantastic. The intermediate change breaks us pretty bad, but long term I'm very excited about this, so thanks again!) |
|
Relatedly, this seems a good reason to do #119040. Any code that has a pointer wraparound problem probably also has a problem with |
… of object" (#120460) Reverts llvm/llvm-project#119225 due to the lack of sanitizer support, large potential of breaking code containing latent UB, non-trivial localization and investigation, and what seems to be a bad interaction with msan (a test is in the works). Related discussions: llvm/llvm-project#119225 (comment) llvm/llvm-project#118472 (comment)
Unsigned icmp of gep nuw folds to unsigned icmp of offsets. Unsigned icmp of gep nusw nuw folds to unsigned samesign icmp of offsets.
Proofs: https://alive2.llvm.org/ce/z/VEwQY8