Skip to content

[PowerPC] Mark llvm.tan on vectors as expand #95507

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

Closed
wants to merge 1 commit into from

Conversation

ecnelises
Copy link
Member

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Jun 14, 2024

@llvm/pr-subscribers-backend-powerpc

Author: Qiu Chaofan (ecnelises)

Changes

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

2 Files Affected:

  • (modified) llvm/lib/Target/PowerPC/PPCISelLowering.cpp (+1)
  • (modified) llvm/test/CodeGen/PowerPC/lower-intrinsics-nofast-mass.ll (+9)
diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
index 12e33ddb8eb53..e922b5d445e68 100644
--- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -860,6 +860,7 @@ PPCTargetLowering::PPCTargetLowering(const PPCTargetMachine &TM,
       setOperationAction(ISD::FEXP2, VT, Expand);
       setOperationAction(ISD::FSIN, VT, Expand);
       setOperationAction(ISD::FCOS, VT, Expand);
+      setOperationAction(ISD::FTAN, VT, Expand);
       setOperationAction(ISD::FABS, VT, Expand);
       setOperationAction(ISD::FFLOOR, VT, Expand);
       setOperationAction(ISD::FCEIL,  VT, Expand);
diff --git a/llvm/test/CodeGen/PowerPC/lower-intrinsics-nofast-mass.ll b/llvm/test/CodeGen/PowerPC/lower-intrinsics-nofast-mass.ll
index 3d6e6da3b66d9..0c49d10e675a7 100644
--- a/llvm/test/CodeGen/PowerPC/lower-intrinsics-nofast-mass.ll
+++ b/llvm/test/CodeGen/PowerPC/lower-intrinsics-nofast-mass.ll
@@ -145,3 +145,12 @@ entry:
   %0 = tail call double @llvm.sin.f64(double %a)
   ret double %0
 }
+
+define <2 x double> @tan_v2f64_nofast(<2 x double> %a) {
+; CHECK-LABEL: tan_v2f64_nofast
+; CHECK-NOT: bl __xl_tan
+; CHECK: blr
+entry:
+  %0 = tail call <2 x double> @llvm.tan.v2f64(<2 x double> %a)
+  ret <2 x double> %0
+}

@farzonl
Copy link
Member

farzonl commented Jun 14, 2024

Hi thanks for picking this up iI've been working on a general fix so I have a list of all the tests that need to be updated. For PowerPC it is:

  • CodeGen/PowerPC/f128-arith.ll
  • CodeGen/PowerPC/lower-intrinsics-afn-mass.ll
  • CodeGen/PowerPC/lower-intrinsics-afn-mass_notail.ll
  • CodeGen/PowerPC/lower-intrinsics-fast-mass.ll
  • CodeGen/PowerPC/lower-intrinsics-fast-mass_notail.ll
  • CodeGen/PowerPC/lower-intrinsics-nofast-mass.ll
  • CodeGen/PowerPC/unal-altivec2.ll
  • CodeGen/PowerPC/lower-intrinsics-fast-mass.ll
  • CodeGen/PowerPC/lower-intrinsics-fast-mass_notail.ll
  • CodeGen/PowerPC/lower-intrinsics-nofast-mass.ll
  • CodeGen/PowerPC/unal-altivec2.ll

Also in llvm/include/llvm/Analysis/VecFuncs.def add

TLI_DEFINE_VECFUNC("llvm.tan.f64", "__tand2", FIXED(2), "_ZGV_LLVM_N2v") 
TLI_DEFINE_VECFUNC("llvm.tan.f32", "__tanf4", FIXED(4), "_ZGV_LLVM_N4v")

@@ -860,6 +860,7 @@ PPCTargetLowering::PPCTargetLowering(const PPCTargetMachine &TM,
setOperationAction(ISD::FEXP2, VT, Expand);
setOperationAction(ISD::FSIN, VT, Expand);
setOperationAction(ISD::FCOS, VT, Expand);
setOperationAction(ISD::FTAN, VT, Expand);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to handle STRICT_FTAN too?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already been handled by common logic, and there's already been test for constrained tan in vector-constrained-fp-intrinsics.ll

@ecnelises
Copy link
Member Author

Hi @farzonl, do you mean this patch is not needed, because you will fix vector version of these math library nodes together?

@farzonl
Copy link
Member

farzonl commented Jun 14, 2024

Hi @farzonl, do you mean this patch is not needed, because you will fix vector version of these math library nodes together?

My plan was to do an expand in TargetLoweringBase.cpp in this pr: #95518 That way I knock out more platforms at once.

I'm doing spot testing to make sure f16 and vector work. Also PowerPC needs llvm vectors to convert to IBM MASS Vector functions and my change doesn't do that either.

@ecnelises ecnelises closed this Dec 8, 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.

4 participants