[InstCombine] Enable select freeze poison folding when storing value #129776
Conversation
This file contains 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
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-ir Author: John McIver (jmciver) ChangesThe non-freeze poison argument to select can be one of the following: global, Alive2 test validation: https://alive2.llvm.org/ce/z/jbtCS6 Full diff: https://github.com/llvm/llvm-project/pull/129776.diff 3 Files Affected:
diff --git a/llvm/include/llvm/IR/PatternMatch.h b/llvm/include/llvm/IR/PatternMatch.h
index b3eeb1d7ba88a..c66235f33cd9e 100644
--- a/llvm/include/llvm/IR/PatternMatch.h
+++ b/llvm/include/llvm/IR/PatternMatch.h
@@ -3124,6 +3124,19 @@ inline auto m_c_LogicalOp(const LHS &L, const RHS &R) {
return m_LogicalOp<LHS, RHS, /*Commutable=*/true>(L, R);
}
+struct GuaranteedNotToBeUndefOrPoison_match {
+ template <typename ITy> bool match(ITy *V) {
+ if (auto *AsValue = dyn_cast<Value>(V))
+ return isGuaranteedNotToBeUndefOrPoison(AsValue);
+ else
+ return false;
+ }
+};
+
+inline GuaranteedNotToBeUndefOrPoison_match m_guaranteedNotToBeUndefOrPoison() {
+ return GuaranteedNotToBeUndefOrPoison_match();
+}
+
} // end namespace PatternMatch
} // end namespace llvm
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 4c14dcfb4d75f..6b9254692a857 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -4813,15 +4813,22 @@ Instruction *InstCombinerImpl::visitFreeze(FreezeInst &I) {
// TODO: This could use getBinopAbsorber() / getBinopIdentity() to avoid
// duplicating logic for binops at least.
auto getUndefReplacement = [&I](Type *Ty) {
- Constant *BestValue = nullptr;
- Constant *NullValue = Constant::getNullValue(Ty);
+ Value *BestValue = nullptr;
+ Value *NullValue = Constant::getNullValue(Ty);
for (const auto *U : I.users()) {
- Constant *C = NullValue;
+ Value *C = NullValue;
if (match(U, m_Or(m_Value(), m_Value())))
C = ConstantInt::getAllOnesValue(Ty);
else if (match(U, m_Select(m_Specific(&I), m_Constant(), m_Value())))
C = ConstantInt::getTrue(Ty);
-
+ else if (I.hasOneUse() &&
+ match(U, m_c_Select(m_Specific(&I),
+ m_guaranteedNotToBeUndefOrPoison()))) {
+ if (match(U->getOperand(1), m_Specific(&I)))
+ C = U->getOperand(2);
+ else
+ C = U->getOperand(1);
+ }
if (!BestValue)
BestValue = C;
else if (BestValue != C)
@@ -4842,8 +4849,11 @@ Instruction *InstCombinerImpl::visitFreeze(FreezeInst &I) {
Constant *C;
if (match(Op0, m_Constant(C)) && C->containsUndefOrPoisonElement()) {
- Constant *ReplaceC = getUndefReplacement(I.getType()->getScalarType());
- return replaceInstUsesWith(I, Constant::replaceUndefsWith(C, ReplaceC));
+ Value *Replace = getUndefReplacement(I.getType()->getScalarType());
+ if (Constant *ReplaceC = dyn_cast<Constant>(Replace))
+ return replaceInstUsesWith(I, Constant::replaceUndefsWith(C, ReplaceC));
+ else
+ return replaceInstUsesWith(I, Replace);
}
// Replace uses of Op with freeze(Op).
diff --git a/llvm/test/Transforms/InstCombine/select.ll b/llvm/test/Transforms/InstCombine/select.ll
index cc25f4ce24d9a..789d98197d878 100644
--- a/llvm/test/Transforms/InstCombine/select.ll
+++ b/llvm/test/Transforms/InstCombine/select.ll
@@ -4904,8 +4904,7 @@ define i32 @src_simplify_2x_at_once_and(i32 %x, i32 %y) {
define void @select_freeze_poison_parameter(ptr noundef %addr.src, ptr %addr.tgt, i1 %cond) {
; CHECK-LABEL: @select_freeze_poison_parameter(
-; CHECK-NEXT: [[ADDR_SRC:%.*]] = select i1 [[COND:%.*]], ptr [[ADDR_SRC1:%.*]], ptr null
-; CHECK-NEXT: store ptr [[ADDR_SRC]], ptr [[ADDR_TGT:%.*]], align 8
+; CHECK-NEXT: store ptr [[ADDR_SRC:%.*]], ptr [[ADDR_TGT:%.*]], align 8
; CHECK-NEXT: ret void
;
%freeze = freeze ptr poison
@@ -4918,8 +4917,7 @@ define void @select_freeze_poison_parameter(ptr noundef %addr.src, ptr %addr.tgt
define void @select_freeze_poison_global(ptr %addr.tgt, i1 %cond) {
; CHECK-LABEL: @select_freeze_poison_global(
-; CHECK-NEXT: [[SELECT_ADDR:%.*]] = select i1 [[COND:%.*]], ptr @glb, ptr null
-; CHECK-NEXT: store ptr [[SELECT_ADDR]], ptr [[ADDR_TGT:%.*]], align 8
+; CHECK-NEXT: store ptr @glb, ptr [[ADDR_TGT:%.*]], align 8
; CHECK-NEXT: ret void
;
%freeze = freeze ptr poison
@@ -4930,8 +4928,7 @@ define void @select_freeze_poison_global(ptr %addr.tgt, i1 %cond) {
define void @select_freeze_poison_constant(ptr %addr.tgt, i1 %cond) {
; CHECK-LABEL: @select_freeze_poison_constant(
-; CHECK-NEXT: [[SELECT_ADDR:%.*]] = select i1 [[COND:%.*]], i32 72, i32 0
-; CHECK-NEXT: store i32 [[SELECT_ADDR]], ptr [[ADDR_TGT:%.*]], align 4
+; CHECK-NEXT: store i32 72, ptr [[ADDR_TGT:%.*]], align 4
; CHECK-NEXT: ret void
;
%freeze = freeze i32 poison
|
dtcxzyw
reviewed
Mar 5, 2025
There was a problem hiding this comment.
We can do this fold in InstSimplify: https://alive2.llvm.org/ce/z/Dm53TP
However, we should wait for the following things before working on more simplifications with freeze poison:
- Remove
freeze poison -> nullcanonicalization in InstCombine - Replace existing undef handling logic with
freeze poison
See also #119884 (comment)
The transform is only valid if the freeze(poison) is one-use. And I don't think that InstSimplify should be doing any use-based checks. So I think InstCombine is the right place for it. |
|
@nikic and @dtcxzyw thanks for the feedback. This patch does bypasses the need for freeze poison -> null canonicalization removal in InstCombine. To provide context I am seeing the lack of store of select freeze poison folding in uninitialized memory semantics work that I am doing with @nlopes. I would like to incorporate this and then once freeze poison -> null canonicalization is removed from InstCombine refactor appropriately. Would this be acceptable? |
I don't mean to block this patch. I just worry that these patches may not be well tested (fuzzers/compile-time tracker/llvm-opt-benchmark) until we remove the canonicalization. |
dtcxzyw
reviewed
Mar 6, 2025
|
@dtcxzyw I was not aware I should be using a fuzzer. What tool would you recommend? From a correctness standpoint the match is relatively narrow and I have discussed with @nlopes extensively. The patch has been tested with bootstrap build of LLVM running all regressions. Additionally I have used the llvm-test-suite for correctness checks. I have sent a request to @nikita asking for compile-time tracker access and can report results once available. I also have used the patch in conjunction with 15+ Phoronix benchmarks. The optimization involving the fold of global is something I have seen in our memory semantics work in testing FFTW. |
|
FWIW, we have been using this patch internally and it helps substancial in a couple of benchmarks. |
jmciver
force-pushed
the
users/jmciver/select-freeze-poison-2
branch
from
99f2168 to
97d6bec
Compare
|
Performance benchmarks are queued; I will post once available. |
dtcxzyw
reviewed
Mar 7, 2025
dtcxzyw
requested changes
Mar 7, 2025
| @@ -4842,8 +4847,11 @@ Instruction *InstCombinerImpl::visitFreeze(FreezeInst &I) { | |||
|
|
|||
| Constant *C; | |||
| if (match(Op0, m_Constant(C)) && C->containsUndefOrPoisonElement()) { | |||
There was a problem hiding this comment.
Can you add some vector tests?
define <2 x i8> @src(i1 %cond, <2 x i8> noundef %y) {
%undef = freeze <2 x i8> <i8 0, i8 poison>
%sel = select i1 %cond, <2 x i8> %y, <2 x i8> %undef
ret <2 x i8> %sel
}
define <2 x i8> @tgt(i1 %cond, <2 x i8> noundef %y) {
ret <2 x i8> %y
}
It is a miscompilation: https://alive2.llvm.org/ce/z/8Q3XYH
There was a problem hiding this comment.
BTW, as it is not suitable to handle this fold under getUndefReplacement, can we move the logic into InstCombinerImpl::visitSelect?
Instruction *InstCombinerImpl::visitSelectInst(SelectInst &SI) {
...
if (match(TrueVal, m_OneUse(m_Freeze(m_Poison()))) &&
isGuaranteedNotToBeUndefOrPoison(FalseVal, &AC, &I, &DT))
return replaceInstUsesWith(FalseVal);
if (match(FalseVal, m_OneUse(m_Freeze(m_Poison()))) &&
isGuaranteedNotToBeUndefOrPoison(TrueVal, &AC, &I, &DT))
return replaceInstUsesWith(TrueVal);
There was a problem hiding this comment.
Thanks for pointing out the miscompilation!
The reason I did not place this logic into InstCombinerImpl::visitSelect was the canonical form produced by InstCombinerImpl::visitFreeze occurs first and thus ruins the match on freeze poison when visitation is performed on the select instruction.
I experimented a bit and derived a two match expression in the lambda function.
Added basic vector tests. Alive2 validation: https://alive2.llvm.org/ce/z/YFA-5S
There was a problem hiding this comment.
the canonical form produced by InstCombinerImpl::visitFreeze occurs first and thus ruins the match on freeze poison when visitation is performed on the select instruction.
Should we remove these canonicalizations first?
jmciver
force-pushed
the
users/jmciver/select-freeze-poison-2
branch
from
97d6bec to
892c18e
Compare
nikic
reviewed
Mar 8, 2025
jmciver
force-pushed
the
users/jmciver/select-freeze-poison-2
branch
from
892c18e to
790de08
Compare
dtcxzyw
reviewed
Mar 10, 2025
jmciver
force-pushed
the
users/jmciver/select-freeze-poison-2
branch
from
790de08 to
dcf20b2
Compare
|
I refactored the select fold into helper getValueReplacement, which can make use of the freeze argument test in the caller: |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
jmciver
force-pushed
the
users/jmciver/select-freeze-poison-2
branch
from
dcf20b2 to
cd63f3a
Compare
nikic
reviewed
Mar 13, 2025
jmciver
force-pushed
the
users/jmciver/select-freeze-poison-2
branch
from
cd63f3a to
b1f9e87
Compare
|
@nikita I refactored the replacement of poison elements in constant vectors to use the first non-poison element. I am not sure if I need to do something for aggregates. |
jmciver
force-pushed
the
users/jmciver/select-freeze-poison-2
branch
from
b1f9e87 to
4b22bc2
Compare
dtcxzyw
reviewed
Mar 19, 2025
This was referenced Mar 19, 2025
nikic
reviewed
Mar 19, 2025
| auto getUndefReplacement = [&I](Type *Ty) { | ||
| Constant *BestValue = nullptr; | ||
| Constant *NullValue = Constant::getNullValue(Ty); | ||
| auto getUndefReplacement = [&I, &AC = this->AC, &DT = this->DT](Type *Ty) { |
There was a problem hiding this comment.
Suggested change
| auto getUndefReplacement = [&I, &AC = this->AC, &DT = this->DT](Type *Ty) { | |
| auto getUndefReplacement = [&](Type *Ty) { |
There was a problem hiding this comment.
What I meant here is to use [&] to capture everything instead of listing all the variables. You can still use AC and DT if you do that.
There was a problem hiding this comment.
My mistake; I will fix that.
jmciver
force-pushed
the
users/jmciver/select-freeze-poison-2
branch
from
4b22bc2 to
d3abfc1
Compare
|
Update as per reviews. Add constant expression vector tests. |
nikic
reviewed
Mar 19, 2025
jmciver
force-pushed
the
users/jmciver/select-freeze-poison-2
branch
2 times, most recently
from
15b33e4 to
7878416
Compare
The non-freeze poison argument to select can be one of the following: global, constant, and noundef arguments. Alive2 test validation: - https://alive2.llvm.org/ce/z/jbtCS6 - https://alive2.llvm.org/ce/z/YFA-5S Refactor replacement of poison elements in constant vectors to use the first non-poison element.
jmciver
force-pushed
the
users/jmciver/select-freeze-poison-2
branch
from
7878416 to
af0ad2f
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/18017 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/14936 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The non-freeze poison argument to select can be one of the following: global,
constant, and noundef arguments.
Alive2 test validation: https://alive2.llvm.org/ce/z/jbtCS6