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

[Transforms] Let amdgcn take advantage of sin(-x) --> -sin(x) #79700

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AtariDreams
Copy link
Contributor

We do it for amdgcn_cos, and we should do it for amdgcn_sin as well.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 27, 2024

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-llvm-transforms

Author: AtariDreams (AtariDreams)

Changes

We do it for amdgcn_cos, and we should do it for amdgcn_sin as well.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp (+3-2)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index a647be2d26c7613..ce54d49323dc6eb 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -2497,11 +2497,12 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
     }
     break;
   }
-  case Intrinsic::sin: {
+  case Intrinsic::sin:
+  case Intrinsic::amdgcn_sin: {
     Value *X;
     if (match(II->getArgOperand(0), m_OneUse(m_FNeg(m_Value(X))))) {
       // sin(-x) --> -sin(x)
-      Value *NewSin = Builder.CreateUnaryIntrinsic(Intrinsic::sin, X, II);
+      Value *NewSin = Builder.CreateUnaryIntrinsic(IID, X, II);
       Instruction *FNeg = UnaryOperator::CreateFNeg(NewSin);
       FNeg->copyFastMathFlags(II);
       return FNeg;

@nikic nikic requested review from arsenm and removed request for nikic January 27, 2024 17:57
Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

Missing test

@AtariDreams
Copy link
Contributor Author

Missing test

Fixed! @arsenm

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

This is the opposite of what we want for codegen, but I believe we undo this already

@yashssh
Copy link
Contributor

yashssh commented Feb 27, 2024

Out of curiosity what's the purpose of doing this? From one of the tests attached It just looks like we are moving fneg from before sin to after.

@arsenm
Copy link
Contributor

arsenm commented May 16, 2024

Why close this?

@arsenm arsenm reopened this May 16, 2024
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.

None yet

4 participants