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

[InstCombine] optimize powi(X,Y)/X with Ofast #67236

Closed
wants to merge 2 commits into from
Closed

Conversation

vfdff
Copy link
Contributor

@vfdff vfdff commented Sep 23, 2023

Try to transform the powi(X, Y) / X into powi(X, Y-1) with Ofast.

For this case, when the Y is 3, then powi(X, 2) is replaced by X * X in the further step.
Fixes #67216

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 23, 2023

@llvm/pr-subscribers-llvm-transforms

Changes

Fixes #67214


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp (+10)
  • (modified) llvm/test/Transforms/InstCombine/powi.ll (+10)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
index dc091ec7c60e8dd..9c13a63e394a1d5 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
@@ -1764,6 +1764,16 @@ Instruction *InstCombinerImpl::visitFDiv(BinaryOperator &I) {
     return replaceInstUsesWith(I, Pow);
   }
 
+  if (I.hasAllowReassoc() && I.hasNoInfs() &&
+      match(Op0, m_OneUse(m_Intrinsic<Intrinsic::powi>(m_Specific(Op1),
+                                                       m_Value(Y))))) {
+    Constant *NegOne = ConstantInt::getAllOnesValue(Y->getType());
+    Value *Y1 = Builder.CreateAdd(Y, NegOne);
+    Type *Types[] = {Op1->getType(), Y1->getType()};
+    Value *Pow = Builder.CreateIntrinsic(Intrinsic::powi, Types, {Op1, Y1}, &I);
+    return replaceInstUsesWith(I, Pow);
+  }
+
   return nullptr;
 }
 
diff --git a/llvm/test/Transforms/InstCombine/powi.ll b/llvm/test/Transforms/InstCombine/powi.ll
index 20fe25c50a3ffc0..d58d66660690769 100644
--- a/llvm/test/Transforms/InstCombine/powi.ll
+++ b/llvm/test/Transforms/InstCombine/powi.ll
@@ -258,3 +258,13 @@ define double @different_types_powi(double %x, i32 %y, i64 %z) {
   %mul = fmul reassoc double %p2, %p1
   ret double %mul
 }
+
+define nofpclass(nan inf) double @fdiv_powi(double %x) {
+; CHECK-LABEL: @fdiv_powi(
+; CHECK-NEXT:    [[DIV:%.*]] = fmul fast double [[X:%.*]], [[X]]
+; CHECK-NEXT:    ret double [[DIV]]
+;
+  %p1 = call fast double @llvm.powi.f64.i32(double %x, i32 3)
+  %div = fdiv fast double %p1, %x
+  ret double %div
+}

@goldsteinn
Copy link
Contributor

Does alive2 handle this?

@goldsteinn
Copy link
Contributor

Can you add an actual summary in the commit message regarding the transform taking place?

@vfdff
Copy link
Contributor Author

vfdff commented Sep 24, 2023

Does alive2 handle this?

The alive2 failed because timeout, https://alive2.llvm.org/ce/z/w8UGok

llvm/test/Transforms/InstCombine/powi.ll Outdated Show resolved Hide resolved
llvm/test/Transforms/InstCombine/powi.ll Outdated Show resolved Hide resolved
@vfdff vfdff force-pushed the PR67216 branch 3 times, most recently from 8006b52 to 7db76b8 Compare September 26, 2023 09:18
@nikic
Copy link
Contributor

nikic commented Oct 8, 2023

Looks OK, but FP is outside my area of expertise, in particular I wouldn't be able to say which FMF flags a transform should or should not require.

@jcranmer-intel
Copy link
Contributor

Yeah, I'm pretty sure that case is even worse for overflow potential than this one.

@vfdff
Copy link
Contributor Author

vfdff commented Oct 10, 2023

Thanks @jcranmer-intel and @nikic very much , I'll remove the redundant check for ninf

@vfdff
Copy link
Contributor Author

vfdff commented Oct 12, 2023

removed the redundant check for ninf

@vfdff vfdff force-pushed the PR67216 branch 2 times, most recently from afcb7cb to ac165f7 Compare October 13, 2023 03:33
@vfdff vfdff requested a review from dtcxzyw October 17, 2023 04:25
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp Outdated Show resolved Hide resolved
llvm/test/Transforms/InstCombine/powi.ll Outdated Show resolved Hide resolved
llvm/test/Transforms/InstCombine/powi.ll Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Oct 17, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

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

LGTM. Waiting for additional approval from @nikic or @jcranmer-intel.

@vfdff vfdff force-pushed the PR67216 branch 2 times, most recently from ae454ef to 356e99d Compare October 20, 2023 12:45
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

Try to transform the powi(X, Y) / X into powi(X, Y-1) with Ofast.
For this case, when the Y is 3, then powi(X, 2) is replaced by X * X in
the further step.

Fixes llvm#67216
vfdff added a commit that referenced this pull request Oct 21, 2023
Try to transform the powi(X, Y) / X into powi(X, Y-1) with Ofast.
For this case, when the Y is 3, then powi(X, 2) is replaced by X * X in
the further step.

Fixes #67216
Reviewed By: dtcxzyw, nikic, jcranmer-intel
@vfdff vfdff closed this Oct 21, 2023
@vfdff vfdff deleted the PR67216 branch October 21, 2023 01:03
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.

optimize pow(x, i1) / pow(x, i2) with fast-math
6 participants