[MergeICmps] Perform dereferenceability check with context - #202884
Merged
Conversation
To support deref-at-point semantics, we need to check dereferenceability with a context instruction. Currently, MergeICmps does the check for each individual load instruction. In this PR, I'm replacing this with a check for all the loads that are part of a chain after they have been collected, so we do the context-sensitive check only once. The choice of context instruction is a bit tricky: Normally, this would just be the first block in the chain (the "entry block"), but it's also possible for the block to "do extra work", in which case it will get split. If this happens, we should be checking at the splitting point, as the extra work might be freeing the pointer. Another question to consider here is whether we need to be concerned about frees at all: After all, the original code will be accessing at least one byte of the two objects, so doesn't that imply that it wasn't freed already? This is indeed the case, as long as allocations cannot shrink. This is something we currently don't allow, but I think it's something we want to allow, so I'm going with the conservative treatment here.
|
@llvm/pr-subscribers-llvm-transforms Author: Nikita Popov (nikic) ChangesTo support deref-at-point semantics, we need to check dereferenceability with a context instruction. Currently, MergeICmps does the check for each individual load instruction. In this PR, I'm replacing this with a check for all the loads that are part of a chain after they have been collected, so we do the context-sensitive check only once. The choice of context instruction is a bit tricky: Normally, this would just be the first block in the chain (the "entry block"), but it's also possible for the block to "do extra work", in which case it will get split. If this happens, we should be checking at the splitting point, as the extra work might be freeing the pointer. Another question to consider here is whether we need to be concerned about frees at all: After all, the original code will be accessing at least one byte of the two objects, so doesn't that imply that it wasn't freed already? This is indeed the case, as long as allocations cannot shrink. This is something we currently don't allow, but I think it's something we want to allow, so I'm going with the conservative treatment here. Full diff: https://github.com/llvm/llvm-project/pull/202884.diff 3 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/MergeICmps.cpp b/llvm/lib/Transforms/Scalar/MergeICmps.cpp
index 4c7c215d3cb3b..f8beaab7c6f5c 100644
--- a/llvm/lib/Transforms/Scalar/MergeICmps.cpp
+++ b/llvm/lib/Transforms/Scalar/MergeICmps.cpp
@@ -162,13 +162,6 @@ static BCEAtom visitICmpLoadOperand(Value *const Val, BaseIdentifier &BaseId) {
return {};
}
- if (!isDereferenceablePointer(Addr, LoadI->getType(), DL)) {
- LLVM_DEBUG(dbgs() << "not dereferenceable\n");
- // We need to make sure that we can do comparison in any order, so we
- // require memory to be unconditionally dereferenceable.
- return {};
- }
-
APInt Offset = APInt(DL.getIndexTypeSizeInBits(Addr->getType()), 0);
Value *Base = Addr;
auto *GEP = dyn_cast<GetElementPtrInst>(Addr);
@@ -225,7 +218,9 @@ class BCECmpBlock {
// Returns true if the non-BCE-cmp instructions can be separated from BCE-cmp
// instructions in the block.
- bool canSplit(AliasAnalysis &AA) const;
+ // SplitAt is set to the instruction before which the block will have to be
+ // split.
+ bool canSplit(AliasAnalysis &AA, Instruction *&SplitAt) const;
// Return true if this all the relevant instructions in the BCE-cmp-block can
// be sunk below this instruction. By doing this, we know we can separate the
@@ -290,9 +285,11 @@ void BCECmpBlock::split(BasicBlock *NewParent, AliasAnalysis &AA) const {
Inst->moveBeforePreserving(*NewParent, NewParent->begin());
}
-bool BCECmpBlock::canSplit(AliasAnalysis &AA) const {
+bool BCECmpBlock::canSplit(AliasAnalysis &AA, Instruction *&SplitAt) const {
+ SplitAt = nullptr;
for (Instruction &Inst : *BB) {
if (!BlockInsts.count(&Inst)) {
+ SplitAt = Inst.getNextNode();
if (!canSinkBCECmpInst(&Inst, AA))
return false;
}
@@ -416,6 +413,8 @@ class BCECmpChain {
BCECmpChain(const std::vector<BasicBlock *> &Blocks, PHINode &Phi,
AliasAnalysis &AA);
+ bool isDereferenceable();
+
bool simplify(const TargetLibraryInfo &TLI, AliasAnalysis &AA,
DomTreeUpdater &DTU);
@@ -430,6 +429,9 @@ class BCECmpChain {
std::vector<ContiguousBlocks> MergedBlocks_;
// The original entry block (before sorting);
BasicBlock *EntryBlock_;
+ // The instruction before which the entry block needs to be split (or null
+ // if no splitting required).
+ Instruction *SplitAt = nullptr;
};
} // namespace
@@ -518,13 +520,14 @@ BCECmpChain::BCECmpChain(const std::vector<BasicBlock *> &Blocks, PHINode &Phi,
// and start anew.
//
// NOTE: we only handle blocks a with single predecessor for now.
- if (Comparison->canSplit(AA)) {
+ if (Comparison->canSplit(AA, SplitAt)) {
LLVM_DEBUG(dbgs()
<< "Split initial block '" << Comparison->BB->getName()
<< "' that does extra work besides compare\n");
Comparison->RequireSplit = true;
enqueueBlock(Comparisons, std::move(*Comparison));
} else {
+ SplitAt = nullptr;
LLVM_DEBUG(dbgs()
<< "ignoring initial block '" << Comparison->BB->getName()
<< "' that does extra work besides compare\n");
@@ -734,6 +737,37 @@ static BasicBlock *mergeComparisons(ArrayRef<BCECmpBlock> Comparisons,
return BB;
}
+// The transform may change the order in which the comparison is performed,
+// in which case we may perform loads that were not performed by the original
+// program. As such, we need to ensure that all the accessed memory is
+// dereferenceable.
+bool BCECmpChain::isDereferenceable() {
+ // We know that there can be no frees inside the merged blocks, so it's
+ // sufficient for dereferenceability to hold at the entry block. One
+ // exception to this is if the entry block performs "other work" and will
+ // get split. In that case, we need to consider frees prior to the splitting
+ // point.
+ Instruction *CxtI = SplitAt ? SplitAt : &EntryBlock_->front();
+
+ for (const auto &Blocks : MergedBlocks_) {
+ const BCECmpBlock &LowestBlock = Blocks.front();
+ const Value *Lhs = LowestBlock.Lhs().LoadI->getPointerOperand();
+ const Value *Rhs = LowestBlock.Rhs().LoadI->getPointerOperand();
+ const DataLayout &DL = LowestBlock.Lhs().LoadI->getDataLayout();
+
+ unsigned SizeInBits = 0;
+ for (const BCECmpBlock &Block : Blocks)
+ SizeInBits += Block.SizeBits();
+
+ APInt Size(64, SizeInBits / 8);
+ SimplifyQuery SQ(DL, CxtI);
+ if (!isDereferenceableAndAlignedPointer(Lhs, Align(1), Size, SQ) ||
+ !isDereferenceableAndAlignedPointer(Rhs, Align(1), Size, SQ))
+ return false;
+ }
+ return true;
+}
+
bool BCECmpChain::simplify(const TargetLibraryInfo &TLI, AliasAnalysis &AA,
DomTreeUpdater &DTU) {
assert(atLeastOneMerged() && "simplifying trivial BCECmpChain");
@@ -888,6 +922,11 @@ static bool processPhi(PHINode &Phi, const TargetLibraryInfo &TLI,
return false;
}
+ if (!CmpChain.isDereferenceable()) {
+ LLVM_DEBUG(dbgs() << "not dereferenceable\n");
+ return false;
+ }
+
return CmpChain.simplify(TLI, AA, DTU);
}
diff --git a/llvm/test/Transforms/MergeICmps/X86/no-gep-other-work.ll b/llvm/test/Transforms/MergeICmps/X86/no-gep-other-work.ll
index 0cbc8dce5bd51..71c040a4a0e8e 100644
--- a/llvm/test/Transforms/MergeICmps/X86/no-gep-other-work.ll
+++ b/llvm/test/Transforms/MergeICmps/X86/no-gep-other-work.ll
@@ -1,5 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt -S -passes=mergeicmps < %s | FileCheck %s
+; RUN: opt -S -passes=mergeicmps < %s | FileCheck %s --check-prefixes=CHECK,GLOBAL
+; RUN: opt -S -passes=mergeicmps -use-dereferenceable-at-point-semantics < %s | FileCheck %s --check-prefixes=CHECK,AT-POINT
; This does not use a GEP for the zero-offset comparison and requires a
; split for other work.
@@ -9,9 +10,57 @@ target triple = "x86_64-grtev4-linux-gnu"
declare void @other_work()
define i1 @test(ptr dereferenceable(2) %arg, ptr dereferenceable(2) %arg1) {
-; CHECK-LABEL: @test(
+; GLOBAL-LABEL: @test(
+; GLOBAL-NEXT: "if+entry":
+; GLOBAL-NEXT: call void @other_work()
+; GLOBAL-NEXT: [[MEMCMP:%.*]] = call i32 @memcmp(ptr [[ARG:%.*]], ptr [[ARG1:%.*]], i64 2)
+; GLOBAL-NEXT: [[TMP0:%.*]] = icmp eq i32 [[MEMCMP]], 0
+; GLOBAL-NEXT: br label [[JOIN:%.*]]
+; GLOBAL: join:
+; GLOBAL-NEXT: ret i1 [[TMP0]]
+;
+; AT-POINT-LABEL: @test(
+; AT-POINT-NEXT: entry:
+; AT-POINT-NEXT: call void @other_work()
+; AT-POINT-NEXT: [[ARG_OFF:%.*]] = getelementptr inbounds i8, ptr [[ARG:%.*]], i64 1
+; AT-POINT-NEXT: [[ARG1_OFF:%.*]] = getelementptr inbounds i8, ptr [[ARG1:%.*]], i64 1
+; AT-POINT-NEXT: [[ARG_OFF_VAL:%.*]] = load i8, ptr [[ARG_OFF]], align 1
+; AT-POINT-NEXT: [[ARG1_OFF_VAL:%.*]] = load i8, ptr [[ARG1_OFF]], align 1
+; AT-POINT-NEXT: [[CMP_OFF:%.*]] = icmp eq i8 [[ARG_OFF_VAL]], [[ARG1_OFF_VAL]]
+; AT-POINT-NEXT: br i1 [[CMP_OFF]], label [[IF:%.*]], label [[JOIN:%.*]]
+; AT-POINT: if:
+; AT-POINT-NEXT: [[ARG_VAL:%.*]] = load i8, ptr [[ARG]], align 1
+; AT-POINT-NEXT: [[ARG1_VAL:%.*]] = load i8, ptr [[ARG1]], align 1
+; AT-POINT-NEXT: [[CMP:%.*]] = icmp eq i8 [[ARG_VAL]], [[ARG1_VAL]]
+; AT-POINT-NEXT: br label [[JOIN]]
+; AT-POINT: join:
+; AT-POINT-NEXT: [[PHI:%.*]] = phi i1 [ false, [[ENTRY:%.*]] ], [ [[CMP]], [[IF]] ]
+; AT-POINT-NEXT: ret i1 [[PHI]]
+;
+entry:
+ call void @other_work()
+ %arg.off = getelementptr inbounds i8, ptr %arg, i64 1
+ %arg1.off = getelementptr inbounds i8, ptr %arg1, i64 1
+ %arg.off.val = load i8, ptr %arg.off
+ %arg1.off.val = load i8, ptr %arg1.off
+ %cmp.off = icmp eq i8 %arg.off.val, %arg1.off.val
+ br i1 %cmp.off, label %if, label %join
+
+if:
+ %arg.val = load i8, ptr %arg
+ %arg1.val = load i8, ptr %arg1
+ %cmp = icmp eq i8 %arg.val, %arg1.val
+ br label %join
+
+join:
+ %phi = phi i1 [ false, %entry ], [ %cmp, %if ]
+ ret i1 %phi
+}
+
+define i1 @test_nofree(ptr dereferenceable(2) %arg, ptr dereferenceable(2) %arg1) {
+; CHECK-LABEL: @test_nofree(
; CHECK-NEXT: "if+entry":
-; CHECK-NEXT: call void @other_work()
+; CHECK-NEXT: call void @other_work() #[[ATTR1:[0-9]+]]
; CHECK-NEXT: [[MEMCMP:%.*]] = call i32 @memcmp(ptr [[ARG:%.*]], ptr [[ARG1:%.*]], i64 2)
; CHECK-NEXT: [[TMP0:%.*]] = icmp eq i32 [[MEMCMP]], 0
; CHECK-NEXT: br label [[JOIN:%.*]]
@@ -19,7 +68,7 @@ define i1 @test(ptr dereferenceable(2) %arg, ptr dereferenceable(2) %arg1) {
; CHECK-NEXT: ret i1 [[TMP0]]
;
entry:
- call void @other_work()
+ call void @other_work() nofree
%arg.off = getelementptr inbounds i8, ptr %arg, i64 1
%arg1.off = getelementptr inbounds i8, ptr %arg1, i64 1
%arg.off.val = load i8, ptr %arg.off
diff --git a/llvm/test/Transforms/MergeICmps/X86/opaque-ptr.ll b/llvm/test/Transforms/MergeICmps/X86/opaque-ptr.ll
index df2fa0cee08dd..f3ba65759f32a 100644
--- a/llvm/test/Transforms/MergeICmps/X86/opaque-ptr.ll
+++ b/llvm/test/Transforms/MergeICmps/X86/opaque-ptr.ll
@@ -1,5 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -S -passes=mergeicmps < %s | FileCheck %s
+; RUN: opt -S -passes=mergeicmps -use-dereferenceable-at-point-semantics < %s | FileCheck %s
target triple = "x86_64-unknown-unknown"
|
| SizeInBits += Block.SizeBits(); | ||
|
|
||
| APInt Size(64, SizeInBits / 8); | ||
| SimplifyQuery SQ(DL, CxtI); |
Contributor
There was a problem hiding this comment.
Would the nature / invariants of the pass allow us not to particularly care about DT (/AC)-based reasoning here when proving dereferenceability?
Contributor
Author
There was a problem hiding this comment.
In theory AC/DT could be useful to support this transform based on assume_dereferenceable. The pass doesn't use AC/DT currently though (DT is only preserved), and I don't think it's worth it to request them just for this.
antoniofrighetto
approved these changes
Jun 12, 2026
antoniofrighetto
left a comment
antoniofrighetto
left a comment
Contributor
There was a problem hiding this comment.
LGTM
This is something we currently don't allow, but I think it's something we want to allow
Do we intend to refine the LangRef to allow allocation shrinking some time soon?
Contributor
Author
Not specific plans for this right now. There is some related discussion on #141338 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
To support deref-at-point semantics, we need to check dereferenceability with a context instruction. Currently, MergeICmps does the check for each individual load instruction. In this PR, I'm replacing this with a check for all the loads that are part of a chain after they have been collected, so we do the context-sensitive check only once.
The choice of context instruction is a bit tricky: Normally, this would just be the first block in the chain (the "entry block"), but it's also possible for the block to "do extra work", in which case it will get split. If this happens, we should be checking at the splitting point, as the extra work might be freeing the pointer.
Another question to consider here is whether we need to be concerned about frees at all: After all, the original code will be accessing at least one byte of the two objects, so doesn't that imply that it wasn't freed already? This is indeed the case, as long as allocations cannot shrink. This is something we currently don't allow, but I think it's something we want to allow, so I'm going with the conservative treatment here.