LGTM

In D128653#3633704, @lenary wrote:

You likely need // REQUIRES: powerpc-registered-target in the top of the test, as -enable-ppc-gen-scalar-mass is only present if the PowerPC target has been compiled into LLVM.

Just took it down temporarily. -- Looking at it...

Update the test

Test updated.

Add more tests.

Ready for another round of review.

Changing function name: lowerLibCall() -> lowerLibCallType()

Fix test cases.

This update will fix the type of arguments passing to the converted math function in PPCISelLowing.cpp.

A gentle reminder for reviewers. -- This patch will fix the bug reported here: https://bugs.llvm.org/show_bug.cgi?id=52565

This PR is created in support for IBM scalar MASS library pass, but with new approach of handling llvm intrinsic math functions on https://reviews.llvm.org/D101759. There is no need for this PR.

As suggested before, I removed dependency to the global option to convert math functions to MASS for all intrinsic and non-intrinsic functions.
The main changes here with respect to the last proposal is in PPCIselLowing.cpp file, about how to handle llvm intrinsic math function.

Gentle reminder for reviewers. -- This PR is ready for review.

Updated the test combining -ffast-math and -fno-approx-func options.
@andrew.w.kaylor I hope you don't mind that I put those tests on clang/test/Driver/fast-math.c instead.

A gentle reminder for reviewers.

Update the documentation.

Update the documentation.

Description and driver test are added.

Reminder for reviewers.

Ready for review.

Sorry that it took me so long to reply reviews. Thank you for reviewing this patch.

Removing dependency to the global option to convert math functions to MASS.

In D101759#2967250, @lebedev.ri wrote:

Do we *really* need -enable-approx-func-fp-math?
I'm pretty sure we are moving away from such global options, onto relying only on the per-instruction fast-math flags.

In D106191#2966852, @qiucf wrote:

Making a new option mapped to another float op flag looks reasonable, but is there any clearer motivation for this? (such as the need for -Ofast -fno-approx-func)

In D106091#2896031, @xgupta wrote:

Seems patch is committed in https://reviews.llvm.org/rGee2068b30ecf297c004c06eedd7e1063c67a279c, revision should be close now?

Remove extra function deceleration.

Removed clang changes from this PR.
Removed extra option for MASS pass.
Now MASS pass is active with -O3 and approx-func option.

Removed dependency to unsafe-fp-math and added clang option to
control afn flag.

Sorry it took me so long to update this patch -- I think I addressed all reviews till now.

Addressed.

Checking sqrtd2_massv and sqrtf4_massv is not generated.

Sorry, I forgot the clang-format.

I added support for the case when exponent is 0.25 in addition to support for double precision cases.

Moving completely the changes from PPCISelLowring.cpp to PPCLowerMASSVEntries.cpp (MASSV pass) to address the reviewer comments.

@steven.zhang
I think I had done a terrible mistake. When I tested with my c code, I didn't have if (ClVectorLibrary == TargetLibraryInfoImpl::MASSV) on

if (ClVectorLibrary == TargetLibraryInfoImpl::MASSV)
  setOperationAction(ISD::FPOW, MVT::v4f32, Custom);

and for some reason if I move this check inside the function LowerFPOWMASSV it works good. So I am updating the patch. Thank you for catching it.

In D80744#2070174, @steven.zhang wrote:

In D80744#2068900, @masoud.ataei wrote:

In D80744#2067821, @steven.zhang wrote:

In D80744#2066590, @masoud.ataei wrote:

In D80744#2062144, @steven.zhang wrote:

What we are doing is as follows:
llvm.pow(IR) --> FPOW(ISD) --> __powf4_P8/9(ISD/IR)

It makes more sense to do it in the IR pass from what I see. And then, you can query the TargetLibraryInfoImpl::isFunctionVectorizable(name) instead of the option. Maybe, we can do it in ppc pass: PPCLowerMASSVEntries which did something like:

__sind2_massv --> __sind2_P9 for a Power9 subtarget.

Does it make sense ?

I agree in general it makes more sense to do this kind of conversions in an IR pass like PPCLowerMASSVEntries. This is what we are currently doing in LLVM but there is a problem here. If we change the llvm intrinsic to libcall earlier than a later optimization (like in DAGCombiner) the later optimization won't be triggered. In the case that I am proposing the change, if we have pow(x,0.75) in the code, the PPCLowerMASSVEntries pass will currently change it to __powf4_P* libcall then later in the DAGCombiner we will not get the optimization pow(x,0.75) --> sqrt(x)*sqrt(sqrt(x)). So that's a problem, because sqrt(x)*sqrt(sqrt(x)) is faster.

What I am proposing is to move the conversion for powf4 to late in the compiler pipeline. With this change we will we will get above optimization when exponent is 0.75 and MASSV calls otherwise.

If we expect the llvm.pow(x, 0.75) to be lowered as two sqrt and for others, they are libcall, can we just don't transform it as libcall for 0.75 in PPCLowerMASSVEntries? And it will be lowered as two sqrts in DAGCombine.

For pow to be __powf4_P8, there are two step:

1. in LoopVectorizePass, pow becomes __powf4_massv, then
2. in PPCLowerMASSVEntries __powf4_massv becomes __powf4_P8

So when we reach PPCLowerMASSVEntries the pow intrinsic is already a libcall. I thought it is really ugly to undo the LoopVectorizePass conversion in PPCLowerMASSVEntries pass when there is an special case.

As what I see from your test is that, we are trying to lower the pow intrinsic(not pow libcall) to two sqrts or __powf4_P8/9. It is different from the case that, pow -> __powf4_massv -> __powf4_P8 as they are all libcall path.

If your intention is to have __powf4_massv transformed to two fsqrts if argument is 0.75, the test is missing in this patch and I am not sure if this patch could work. You might need to transform the __powf4_massv to llvm.pow intrinsic at some place like PartiallyInlineLibCallsLegacyPass if the argument is 0.75

Maybe, I miss some background here and I just want to get a clear understanding of the reason why we have to do it in DAG. Thank you for your patience.

In D80744#2067821, @steven.zhang wrote:

In D80744#2066590, @masoud.ataei wrote:

In D80744#2062144, @steven.zhang wrote:

What we are doing is as follows:
llvm.pow(IR) --> FPOW(ISD) --> __powf4_P8/9(ISD/IR)

It makes more sense to do it in the IR pass from what I see. And then, you can query the TargetLibraryInfoImpl::isFunctionVectorizable(name) instead of the option. Maybe, we can do it in ppc pass: PPCLowerMASSVEntries which did something like:

__sind2_massv --> __sind2_P9 for a Power9 subtarget.

Does it make sense ?

I agree in general it makes more sense to do this kind of conversions in an IR pass like PPCLowerMASSVEntries. This is what we are currently doing in LLVM but there is a problem here. If we change the llvm intrinsic to libcall earlier than a later optimization (like in DAGCombiner) the later optimization won't be triggered. In the case that I am proposing the change, if we have pow(x,0.75) in the code, the PPCLowerMASSVEntries pass will currently change it to __powf4_P* libcall then later in the DAGCombiner we will not get the optimization pow(x,0.75) --> sqrt(x)*sqrt(sqrt(x)). So that's a problem, because sqrt(x)*sqrt(sqrt(x)) is faster.

What I am proposing is to move the conversion for powf4 to late in the compiler pipeline. With this change we will we will get above optimization when exponent is 0.75 and MASSV calls otherwise.

If we expect the llvm.pow(x, 0.75) to be lowered as two sqrt and for others, they are libcall, can we just don't transform it as libcall for 0.75 in PPCLowerMASSVEntries? And it will be lowered as two sqrts in DAGCombine.

In D80744#2062144, @steven.zhang wrote:

What we are doing is as follows:
llvm.pow(IR) --> FPOW(ISD) --> __powf4_P8/9(ISD/IR)

It makes more sense to do it in the IR pass from what I see. And then, you can query the TargetLibraryInfoImpl::isFunctionVectorizable(name) instead of the option. Maybe, we can do it in ppc pass: PPCLowerMASSVEntries which did something like:

__sind2_massv --> __sind2_P9 for a Power9 subtarget.

Does it make sense ?

Addressing the reviews