[InstCombine] Add fold for fabs(-x) -> fabs(x) #94183

VaibhavRumale · 2024-06-03T04:12:44Z

Here is my patch for #94170

Added the missing optimization in visitCallInst function to fold fabs(-x) into `fabs(x)'. I also added the required test cases to verify the optimization for various data types, including scalar floats, doubles, and vector types.

github-actions · 2024-06-03T04:13:02Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-06-03T04:13:32Z

@llvm/pr-subscribers-llvm-transforms

Author: Vaibhav (VaibhavRumale)

Changes

Here is my patch for #94170

Added the missing optimization in visitCallInst function to fold fabs(-x) into `fabs(x)'. I also added the required test cases to verify the optimization for various data types, including scalar floats, doubles, and vector types.

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp (+7)
(added) llvm/test/Transforms/InstCombine/fabs-fneg-fold.ll (+57)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index b6f339da31f7f..1e06296c7721f 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -2517,6 +2517,13 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
   }
   case Intrinsic::fabs: {
     Value *Cond, *TVal, *FVal;
+    Value* Arg = II->getArgOperand(0);
+    Value* X;
+    // fabs (-X) --> fabs (X)
+    if (match(Arg, m_FNeg(m_Value(X)))) {
+        return replaceInstUsesWith(CI, Builder.CreateUnaryIntrinsic(Intrinsic::fabs, X));
+    }
+
     if (match(II->getArgOperand(0),
               m_Select(m_Value(Cond), m_Value(TVal), m_Value(FVal)))) {
       // fabs (select Cond, TrueC, FalseC) --> select Cond, AbsT, AbsF
diff --git a/llvm/test/Transforms/InstCombine/fabs-fneg-fold.ll b/llvm/test/Transforms/InstCombine/fabs-fneg-fold.ll
new file mode 100644
index 0000000000000..f40dbec52e91b
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/fabs-fneg-fold.ll
@@ -0,0 +1,57 @@
+; RUN: opt -S -passes=instcombine %s | FileCheck %s
+
+define float @fabs_fneg_f32(float %x) {
+; CHECK-LABEL: define float @fabs_fneg_f32(
+; CHECK-NEXT:    [[FABS:%.*]] = call float @llvm.fabs.f32(float [[X:%.*]])
+; CHECK-NEXT:    ret float [[FABS]]
+;
+  %neg = fneg float %x
+  %fabs = call float @llvm.fabs.f32(float %neg)
+  ret float %fabs
+}
+
+define <2 x float> @fabs_fneg_v2f32(<2 x float> %x) {
+; CHECK-LABEL: define <2 x float> @fabs_fneg_v2f32(
+; CHECK-NEXT:    [[FABS:%.*]] = call <2 x float> @llvm.fabs.v2f32(<2 x float> [[X:%.*]])
+; CHECK-NEXT:    ret <2 x float> [[FABS]]
+;
+  %neg = fneg <2 x float> %x
+  %fabs = call <2 x float> @llvm.fabs.v2f32(<2 x float> %neg)
+  ret <2 x float> %fabs
+}
+
+define double @fabs_fneg_f64(double %x) {
+; CHECK-LABEL: define double @fabs_fneg_f64(
+; CHECK-NEXT:    [[FABS:%.*]] = call double @llvm.fabs.f64(double [[X:%.*]])
+; CHECK-NEXT:    ret double [[FABS]]
+;
+  %neg = fneg double %x
+  %fabs = call double @llvm.fabs.f64(double %neg)
+  ret double %fabs
+}
+
+define <4 x double> @fabs_fneg_v4f64(<4 x double> %x) {
+; CHECK-LABEL: define <4 x double> @fabs_fneg_v4f64(
+; CHECK-NEXT:    [[FABS:%.*]] = call <4 x double> @llvm.fabs.v4f64(<4 x double> [[X:%.*]])
+; CHECK-NEXT:    ret <4 x double> [[FABS]]
+;
+  %neg = fneg <4 x double> %x
+  %fabs = call <4 x double> @llvm.fabs.v4f64(<4 x double> %neg)
+  ret <4 x double> %fabs
+}
+
+define half @fabs_fneg_f16(half %x) {
+; CHECK-LABEL: define half @fabs_fneg_f16(
+; CHECK-NEXT:    [[FABS:%.*]] = call half @llvm.fabs.f16(half [[X:%.*]])
+; CHECK-NEXT:    ret half [[FABS]]
+;
+  %neg = fneg half %x
+  %fabs = call half @llvm.fabs.f16(half %neg)
+  ret half %fabs
+}
+
+declare float @llvm.fabs.f32(float)
+declare <2 x float> @llvm.fabs.v2f32(<2 x float>)
+declare double @llvm.fabs.f64(double)
+declare <4 x double> @llvm.fabs.v4f64(<4 x double>)
+declare half @llvm.fabs.f16(half)

dtcxzyw · 2024-06-03T09:03:48Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

@@ -2517,6 +2517,13 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
  }
  case Intrinsic::fabs: {
    Value *Cond, *TVal, *FVal;
+    Value* Arg = II->getArgOperand(0);


Can you replace all following uses of II->getArgOperand(0) with Arg?

dtcxzyw · 2024-06-03T09:04:22Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

+    Value* Arg = II->getArgOperand(0);
+    Value* X;
+    // fabs (-X) --> fabs (X)
+    if (match(Arg, m_FNeg(m_Value(X)))) {


Please drop the braces.
See https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

dtcxzyw · 2024-06-03T09:05:01Z

llvm/test/Transforms/InstCombine/fabs-fneg-fold.ll

@@ -0,0 +1,57 @@
+; RUN: opt -S -passes=instcombine %s | FileCheck %s


Please regenerate check lines with llvm/utils/update_test_checks.py

dtcxzyw · 2024-06-03T09:05:30Z

llvm/test/Transforms/InstCombine/fabs-fneg-fold.ll

+  ret half %fabs
+}
+
+declare float @llvm.fabs.f32(float)


These declarations are unnecessary.

PR Link: llvm/llvm-project#94183

github-actions · 2024-06-03T09:08:45Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 718331f55529469586c99a55e4b382a1c7485842 73d52ca113af478a784042d7317e48d7978a9e27 -- llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index 1e06296c77..fbcbe77d79 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -2517,11 +2517,12 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
   }
   case Intrinsic::fabs: {
     Value *Cond, *TVal, *FVal;
-    Value* Arg = II->getArgOperand(0);
-    Value* X;
+    Value *Arg = II->getArgOperand(0);
+    Value *X;
     // fabs (-X) --> fabs (X)
     if (match(Arg, m_FNeg(m_Value(X)))) {
-        return replaceInstUsesWith(CI, Builder.CreateUnaryIntrinsic(Intrinsic::fabs, X));
+      return replaceInstUsesWith(
+          CI, Builder.CreateUnaryIntrinsic(Intrinsic::fabs, X));
     }
 
     if (match(II->getArgOperand(0),

goldsteinn · 2024-06-03T15:19:38Z

Thank you for the contribution.

You might want to see: https://llvm.org/docs/InstCombineContributorGuide.html

Particularly in the case of this PR, you are missing Alive2 Proofs and should Pre-Commit your tests.

arsenm · 2024-06-03T16:16:04Z

llvm/test/Transforms/InstCombine/fabs-fneg-fold.ll

+  %fabs = call half @llvm.fabs.f16(half %neg)
+  ret half %fabs
+}
+


Should test flag preservation behavior

arsenm · 2024-06-03T16:16:50Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

+    Value* Arg = II->getArgOperand(0);
+    Value* X;
+    // fabs (-X) --> fabs (X)
+    if (match(Arg, m_FNeg(m_Value(X)))) {


Somewhere is a strip sign operations helper function that also covers copysign

dtcxzyw · 2024-06-04T08:56:30Z

@VaibhavRumale Please don't update your branch if there is no merge conflict. See https://llvm.org/docs/GitHub.html#rebasing-pull-requests-and-force-pushes

dtcxzyw · 2024-06-04T08:59:39Z

@tstellar Should we turn off Always suggest updating pull request branches? I think it is misleading to new contributors.

VaibhavRumale · 2024-06-04T09:07:46Z

@dtcxzyw Apologies! I will be more careful going forward. Thank you!

arsenm

Should test flag preservation behavior. Should also add similar copysign tests, since those should be handled just like fabs

VaibhavRumale · 2024-06-07T07:53:07Z

@VaibhavRumale Please don't update your branch if there is no merge conflict. See https://llvm.org/docs/GitHub.html#rebasing-pull-requests-and-force-pushes

Thanks for sharing the guide. It seems like I made a mistake of committing from my main branch. Can I create a new PR with my new changes, as I know we cannot use forced commits?

arsenm · 2024-06-07T08:19:45Z

Can I create a new PR with my new changes, as I know we cannot use forced commits?

It's not that you can't, it's just generally discouraged but not always. But you can always create a new one

VaibhavRumale · 2024-06-15T02:39:35Z

Thanks for the reviews. I have opened a new PR #95627 with the changes.

[InstCombine] Add fold for fabs(-x) -> fabs(x)

9461cbc

VaibhavRumale requested a review from nikic as a code owner June 3, 2024 04:12

llvmbot added the llvm:transforms label Jun 3, 2024

Merge branch 'main' into main

73d52ca

dtcxzyw reviewed Jun 3, 2024

View reviewed changes

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Jun 3, 2024

pre-commit: test PR94183

0894442

PR Link: llvm/llvm-project#94183

dtcxzyw mentioned this pull request Jun 3, 2024

pre-commit: test PR94183 dtcxzyw/llvm-opt-benchmark#655

Closed

dtcxzyw requested a review from arsenm June 3, 2024 09:10

nikic added the floating-point Floating-point math label Jun 3, 2024

nikic removed their request for review June 3, 2024 09:10

arsenm reviewed Jun 3, 2024

View reviewed changes

Merge branch 'llvm:main' into main

267f9b5

arsenm requested changes Jun 6, 2024

View reviewed changes

VaibhavRumale closed this by deleting the head repository Jun 14, 2024

dtcxzyw mentioned this pull request Jun 15, 2024

[InstCombine] Add fold for fabs(-x) -> fabs(x) #95627

Merged

		@@ -0,0 +1,57 @@
		; RUN: opt -S -passes=instcombine %s \| FileCheck %s

[InstCombine] Add fold for fabs(-x) -> fabs(x) #94183

[InstCombine] Add fold for fabs(-x) -> fabs(x) #94183

Conversation

VaibhavRumale commented Jun 3, 2024

Uh oh!

github-actions bot commented Jun 3, 2024

Uh oh!

llvmbot commented Jun 3, 2024

Uh oh!

dtcxzyw Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

dtcxzyw Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

dtcxzyw Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

dtcxzyw Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 3, 2024

Uh oh!

goldsteinn commented Jun 3, 2024

Uh oh!

arsenm Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

dtcxzyw commented Jun 4, 2024

Uh oh!

dtcxzyw commented Jun 4, 2024

Uh oh!

VaibhavRumale commented Jun 4, 2024

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

VaibhavRumale commented Jun 7, 2024

Uh oh!

arsenm commented Jun 7, 2024

Uh oh!

VaibhavRumale commented Jun 15, 2024

Uh oh!

Uh oh!