-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[InstCombine] Treat identical operands as one in pushFreezeToPreventPoisonFromPropagating #145348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[InstCombine] Treat identical operands as one in pushFreezeToPreventPoisonFromPropagating #145348
Conversation
…oisonFromPropagating To push a freeze through an instruction, only one operand may produce poison. However, this currently fails for identical operands which are treated as separate. This patch fixes this by treating them as a single operand.
@llvm/pr-subscribers-llvm-transforms Author: Cullen Rhodes (c-rhodes) ChangesTo push a freeze through an instruction, only one operand may produce Thanks to @david-arm for help with this. Full diff: https://github.com/llvm/llvm-project/pull/145348.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index ce42029261359..e33cf699395f0 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -4774,7 +4774,9 @@ InstCombinerImpl::pushFreezeToPreventPoisonFromPropagating(FreezeInst &OrigFI) {
Use *MaybePoisonOperand = nullptr;
for (Use &U : OrigOpInst->operands()) {
if (isa<MetadataAsValue>(U.get()) ||
- isGuaranteedNotToBeUndefOrPoison(U.get()))
+ isGuaranteedNotToBeUndefOrPoison(U.get()) ||
+ // Treat identical operands as a single operand.
+ (MaybePoisonOperand && MaybePoisonOperand->get() == U.get()))
continue;
if (!MaybePoisonOperand)
MaybePoisonOperand = &U;
diff --git a/llvm/test/Transforms/InstCombine/freeze.ll b/llvm/test/Transforms/InstCombine/freeze.ll
index 8875ce1c566f3..abe7117b27f8c 100644
--- a/llvm/test/Transforms/InstCombine/freeze.ll
+++ b/llvm/test/Transforms/InstCombine/freeze.ll
@@ -142,6 +142,17 @@ define i32 @early_freeze_test3(i32 %v1) {
ret i32 %v4.fr
}
+define i32 @early_freeze_test4(i32 %v1) {
+; CHECK-LABEL: @early_freeze_test4(
+; CHECK-NEXT: [[V2_FR:%.*]] = freeze i32 [[V2:%.*]]
+; CHECK-NEXT: [[V3:%.*]] = mul i32 [[V2_FR]], [[V2_FR]]
+; CHECK-NEXT: ret i32 [[V3]]
+;
+ %v2 = mul i32 %v1, %v1
+ %v2.fr = freeze i32 %v2
+ ret i32 %v2.fr
+}
+
; If replace all dominated uses of v to freeze(v).
define void @freeze_dominated_uses_test1(i32 %v) {
|
isGuaranteedNotToBeUndefOrPoison(U.get())) | ||
isGuaranteedNotToBeUndefOrPoison(U.get()) || | ||
// Treat identical operands as a single operand. | ||
(MaybePoisonOperand && MaybePoisonOperand->get() == U.get())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below is still only replacing one of the uses. We should be replacing all of them.
You kind of get away with it because we'll later try to freeze other uses in a separate transform, but this needs to be individually correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably switch this code from working on Use* to working on Value*.
To push a freeze through an instruction, only one operand may produce
poison. However, this currently fails for identical operands which are
treated as separate. This patch fixes this by treating them as a single
operand.
Thanks to @david-arm for help with this.