Skip to content
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

[FuncSpec] Improve estimation of select instruction. #111176

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

labrinea
Copy link
Collaborator

@labrinea labrinea commented Oct 4, 2024

When propagating a constant to a select instruction we only consider the condition operand as the use. I am extending the logic to consider the true and false values too, in case the condition had been found to be constant in a previous propagation but halted.

When propagating a constant to a select instruction we only consider
the condition operand as the use. I am extending the logic to consider
the true and false values too, in case the condition had been found
to be constant in a previous propagation but halted.
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 4, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Alexandros Lamprineas (labrinea)

Changes

When propagating a constant to a select instruction we only consider the condition operand as the use. I am extending the logic to consider the true and false values too, in case the condition had been found to be constant in a previous propagation but halted.


Full diff: https://github.com/llvm/llvm-project/pull/111176.diff

1 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/FunctionSpecialization.cpp (+10-7)
diff --git a/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp b/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
index 548335d750e33d..7d109af9091479 100644
--- a/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
@@ -423,13 +423,16 @@ Constant *InstCostVisitor::visitGetElementPtrInst(GetElementPtrInst &I) {
 Constant *InstCostVisitor::visitSelectInst(SelectInst &I) {
   assert(LastVisited != KnownConstants.end() && "Invalid iterator!");
 
-  if (I.getCondition() != LastVisited->first)
-    return nullptr;
-
-  Value *V = LastVisited->second->isZeroValue() ? I.getFalseValue()
-                                                : I.getTrueValue();
-  Constant *C = findConstantFor(V, KnownConstants);
-  return C;
+  if (I.getCondition() == LastVisited->first) {
+    Value *V = LastVisited->second->isZeroValue() ? I.getFalseValue()
+                                                  : I.getTrueValue();
+    return findConstantFor(V, KnownConstants);
+  }
+  if (Constant *Condition = findConstantFor(I.getCondition(), KnownConstants))
+    if (I.getTrueValue() == LastVisited->first && Condition->isOneValue() ||
+        I.getFalseValue() == LastVisited->first && Condition->isZeroValue())
+      return LastVisited->second;
+  return nullptr;
 }
 
 Constant *InstCostVisitor::visitCastInst(CastInst &I) {

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 4, 2024

@llvm/pr-subscribers-function-specialization

Author: Alexandros Lamprineas (labrinea)

Changes

When propagating a constant to a select instruction we only consider the condition operand as the use. I am extending the logic to consider the true and false values too, in case the condition had been found to be constant in a previous propagation but halted.


Full diff: https://github.com/llvm/llvm-project/pull/111176.diff

1 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/FunctionSpecialization.cpp (+10-7)
diff --git a/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp b/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
index 548335d750e33d..7d109af9091479 100644
--- a/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
@@ -423,13 +423,16 @@ Constant *InstCostVisitor::visitGetElementPtrInst(GetElementPtrInst &I) {
 Constant *InstCostVisitor::visitSelectInst(SelectInst &I) {
   assert(LastVisited != KnownConstants.end() && "Invalid iterator!");
 
-  if (I.getCondition() != LastVisited->first)
-    return nullptr;
-
-  Value *V = LastVisited->second->isZeroValue() ? I.getFalseValue()
-                                                : I.getTrueValue();
-  Constant *C = findConstantFor(V, KnownConstants);
-  return C;
+  if (I.getCondition() == LastVisited->first) {
+    Value *V = LastVisited->second->isZeroValue() ? I.getFalseValue()
+                                                  : I.getTrueValue();
+    return findConstantFor(V, KnownConstants);
+  }
+  if (Constant *Condition = findConstantFor(I.getCondition(), KnownConstants))
+    if (I.getTrueValue() == LastVisited->first && Condition->isOneValue() ||
+        I.getFalseValue() == LastVisited->first && Condition->isZeroValue())
+      return LastVisited->second;
+  return nullptr;
 }
 
 Constant *InstCostVisitor::visitCastInst(CastInst &I) {

@hazzlim
Copy link
Contributor

hazzlim commented Oct 8, 2024

This seems like a worthwhile improvement to the logic here. Might it be useful to have some test coverage for this change?

* Added surrounding parentheses in condition to fix warning.
* Added a unittest.
Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@labrinea labrinea merged commit 6472cb1 into llvm:main Oct 9, 2024
9 checks passed
@labrinea labrinea deleted the funcspec-visit-select branch October 9, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants