Just one question otherwise patch looks good to me.

I tried to build the patch on top of the latest trunk and facing this issue.

The patch looks good to me. But please wait for the approval from other vectorization experts in the reviewer list.

Hi Florian,

Hi Florian,
--Snip--
void foo(double * restrict a, double * restrict b, int N) {
int k=0;
for(int i = 0 , k =2; i < N; ++i, ++k)

a[i] = sin(i)+k;

}
--Snip--

In D95373#2525539, @LukasSommerTu wrote:

Thanks to everyone for your feedback so far!

I haven't actually looked at the patch details, but if there's agreement that it's ok to leave it as an IR transform, then it should be a small adjustment to make this an IR codegen pass. It would be similar to the existing ExpandReductions pass.

As this solution (i.e., keeping it as an IR pass, but move it to be an IR codegen pass) seems to be acceptable, I would change the implementation accordingly and update the patch.

@lebedev.ri, @fhahn, @spatel, @venkataramanan.kumar.llvm, @craig.topper: Any objections?

In D95373#2523383, @spatel wrote:

In D95373#2523207, @craig.topper wrote:

In D95373#2522607, @fhahn wrote:

In D95373#2522603, @spatel wrote:

I haven't looked at the trade-offs, but if we can do this transform later ( SelectionDAGLegalize::ConvertNodeToLibcall() ? ) that would seem to be more flexible...and smaller patch?
For reference:
D8131 - added support for external vector library calls
D70107 - added inject-tli-mappings

+1 on doing this as late as possible. Ideally we would then just get rid of the logic in LV/SLP that inserts the library calls.

I think SelectionDAG would scalarize the operation in LegalizeVectorOps before they reach LegalizeDAG.

So an IR codegen pass that runs after the optimizer would be best? If the patch is small enough, we could glom this into CGP?

In D88712#2414171, @spatel wrote:

In D88712#2413877, @venkataramanan.kumar.llvm wrote:

In D88712#2413688, @spatel wrote:

In D88712#2412874, @venkataramanan.kumar.llvm wrote:

In D88712#2412366, @MaskRay wrote:

In D88712#2411841, @venkataramanan.kumar.llvm wrote:

For your example:

extern double log (double) asm ("" "log_finite") __attribute__ ((nothrow));

double mylog (double d) { return log(d); }

The intention is to emit log_finite, no matter the -ffast-math. Both GCC and Clang (after this patch) emit jmp log_finite.

The previous behavior (according to your comment, with an older glibc) was undesired. So I think the right suggestion is to upgrade glibc.

Then there optimizations like vectorization, inst combine which works on the LLVM intrinsics. But they will be not happening now with log_finite calls.

I'm not sure about the expected semantics/lowering for the finite calls, but can you add something under LibCallSimplifier::optimizeFloatingPointLibCall() that would turn it into the LLVM log intrinsic with appropriate FMF? Codegen would need to be responsible for converting it back to finite call(s) if those are available?

Hi Sanjay I thought codegen no longer lowers them back to finite calls
https://reviews.llvm.org/rGcd0926d087a85c5ee1222ca80980b4440214a822

There was a comment in D74712 that we might be moving too fast. If you would like to revert/adjust that patch, raise a bug or post a patch to discuss? If the goal is to be able to vectorize the finite calls, then we will need some way to represent those. Alternatively, we could change something in the cost models to enable more unrolling, etc?

In D88712#2413688, @spatel wrote:

In D88712#2412874, @venkataramanan.kumar.llvm wrote:

In D88712#2412366, @MaskRay wrote:

In D88712#2411841, @venkataramanan.kumar.llvm wrote:

For your example:

extern double log (double) asm ("" "log_finite") __attribute__ ((nothrow));

double mylog (double d) { return log(d); }

The intention is to emit log_finite, no matter the -ffast-math. Both GCC and Clang (after this patch) emit jmp log_finite.

The previous behavior (according to your comment, with an older glibc) was undesired. So I think the right suggestion is to upgrade glibc.

Then there optimizations like vectorization, inst combine which works on the LLVM intrinsics. But they will be not happening now with log_finite calls.

I'm not sure about the expected semantics/lowering for the finite calls, but can you add something under LibCallSimplifier::optimizeFloatingPointLibCall() that would turn it into the LLVM log intrinsic with appropriate FMF? Codegen would need to be responsible for converting it back to finite call(s) if those are available?

In D88712#2412366, @MaskRay wrote:

In D88712#2411841, @venkataramanan.kumar.llvm wrote:

For your example:

extern double log (double) asm ("" "log_finite") __attribute__ ((nothrow));

double mylog (double d) { return log(d); }

The intention is to emit log_finite, no matter the -ffast-math. Both GCC and Clang (after this patch) emit jmp log_finite.

The previous behavior (according to your comment, with an older glibc) was undesired. So I think the right suggestion is to upgrade glibc.

I am on Ubuntu 18 machine and it has finite math header <bits/math-finite.h>.
This header is included by the glibc 2.27. This header has this following definition.
extern double log (double) asm ("" "log_finite") attribute__ ((nothrow ));

Thanks @spatel , @Florian and @fpetrogalli for the review comments and approval. Can someone please commit it on my behalf.

Remove an incorrect file that got attached with my earlier patch.

Added a test case for testing vector library calls for VF=2 and VF=8.

Updated the patch as per review comments received.

As per review comments from Sanjay, updated the test case to use metadata. Also autogenerated the checks in the test cases using llvm/utils/update_test_checks.py.

Changed library naming to LIBMVEC-X86 as per comments and also selected based on Target Tripple in clang.
I am still working on auto generating FileCheck for the test cases.

In D88154#2314352, @spatel wrote:

In D88154#2310653, @venkataramanan.kumar.llvm wrote:

In D88154#2290205, @abique wrote:

Looks good to me.
Regarding the tests, it seems that you check if auto-vectorization takes advantages of libmvec?
Would it be interesting to have a test which declares a vector and call the builtin sin on it?

Thank you very much for the changes! :)

do we we have built-in support for sin that takes vector types?

I tried

m128d compute_sin(m128d x)
{

return __builtin_sin(x);

}

error: passing '__m128d' (vector of 2 'double' values) to parameter of incompatible type 'double'

We have LLVM intrinsics for sin/cos that may use vector types:
http://llvm.org/docs/LangRef.html#llvm-sin-intrinsic
...but I don't know of a way to produce those directly from C source.

Pinging for review comments.

In D88154#2290205, @abique wrote:

Looks good to me.
Regarding the tests, it seems that you check if auto-vectorization takes advantages of libmvec?
Would it be interesting to have a test which declares a vector and call the builtin sin on it?

Thank you very much for the changes! :)

Selection of Glibc vector math library is enabled via the option -fvec-lib=libmvec .

Initial version I supported the following vector functions (VF 2 and 4 ).

Updated the patch as per comments received from Sanjay.

Updated the patch with "nsz" check .

Update the patch as per the review comments given by Sanjay.

Added comment in the test case.

As per the comment given by Sanjay, updated the coverage test for x*1/sqrt(x) pattern.

Folding is not happening when the number of uses for the divide operand (say 1.0/something) is more than one.

Updated patch as per the comments from Sanjay.

Updated the patch as per the comments given by Sanjay. Adjusted the test cases.

Hi Sanjay,

In D85709#2215718, @spatel wrote:

In D85709#2214967, @venkataramanan.kumar.llvm wrote:

In D85709#2211204, @spatel wrote:

After looking at the codegen, I'm not sure if we can do this transform in IR with the expected performance in codegen because the transform loses information:
https://godbolt.org/z/7b84rG

The codegen for the case of "sqrt(x)" has to account for a 0.0 input. Ie, we filter out a 0.0 (or potentially denorm) input to avoid the NAN answer that we would get from "0.0 / 0.0". But the codegen for the case of "x/sqrt(x)" does not have to do that - NAN is the correct answer for a 0.0 input, so the code has implicitly signaled to us that 0.0 is not a valid input when compiled with -ffast-math (we can ignore possible NANs).

It might help to see the motivating code that produces the x/sqrt(x) pattern to see if there's something else we should be doing there.

Current AMD "x86_64" targets don't have the reciprocal sqrt instruction for the double precision types.
so x/sqrt(x) ends up with "vsqrtsd" followed by "vdivsd". This transform is basically to improve the efficiency.

Ah, I see. I think we should handle that in generic DAGCombiner then. There, we can make the target- and CPU-specific trade-offs necessary to get the (presumably) ideal asm code. I don't know how we would recover the missing div-by-0 info that I mentioned here.
Let me know if you want to try that patch. If not, I can take a shot at it.

In D85709#2215718, @spatel wrote:

In D85709#2214967, @venkataramanan.kumar.llvm wrote:

In D85709#2211204, @spatel wrote:

After looking at the codegen, I'm not sure if we can do this transform in IR with the expected performance in codegen because the transform loses information:
https://godbolt.org/z/7b84rG

The codegen for the case of "sqrt(x)" has to account for a 0.0 input. Ie, we filter out a 0.0 (or potentially denorm) input to avoid the NAN answer that we would get from "0.0 / 0.0". But the codegen for the case of "x/sqrt(x)" does not have to do that - NAN is the correct answer for a 0.0 input, so the code has implicitly signaled to us that 0.0 is not a valid input when compiled with -ffast-math (we can ignore possible NANs).

It might help to see the motivating code that produces the x/sqrt(x) pattern to see if there's something else we should be doing there.

Current AMD "x86_64" targets don't have the reciprocal sqrt instruction for the double precision types.
so x/sqrt(x) ends up with "vsqrtsd" followed by "vdivsd". This transform is basically to improve the efficiency.

Ah, I see. I think we should handle that in generic DAGCombiner then. There, we can make the target- and CPU-specific trade-offs necessary to get the (presumably) ideal asm code. I don't know how we would recover the missing div-by-0 info that I mentioned here.
Let me know if you want to try that patch. If not, I can take a shot at it.

In D85709#2211204, @spatel wrote:

After looking at the codegen, I'm not sure if we can do this transform in IR with the expected performance in codegen because the transform loses information:
https://godbolt.org/z/7b84rG

The codegen for the case of "sqrt(x)" has to account for a 0.0 input. Ie, we filter out a 0.0 (or potentially denorm) input to avoid the NAN answer that we would get from "0.0 / 0.0". But the codegen for the case of "x/sqrt(x)" does not have to do that - NAN is the correct answer for a 0.0 input, so the code has implicitly signaled to us that 0.0 is not a valid input when compiled with -ffast-math (we can ignore possible NANs).

It might help to see the motivating code that produces the x/sqrt(x) pattern to see if there's something else we should be doing there.

In D85709#2210125, @spatel wrote:

In D85709#2210042, @cameron.mcinally wrote:

In D85709#2210034, @lebedev.ri wrote:

In D85709#2209979, @cameron.mcinally wrote:

I'm fairly sure this transform is a performance loss. For a target like Skylake Server, a SQRT(x) can take up to 20 cycles. But a RSQRT(x) is about 6 cycles and a MUL(y) is 4 cycles. We'd be better off with a X*RSQRT(X).

That is up to backends to decide. InstSimplify/InstCombine (and a few others) are canonicalization, target-independent passes.
A single sqrt(x) is more canonical IR than x/sqrt(x), because it's less instructions and x has less uses.

I agree with that. It should be canonicalized. It would also be good to make sure that the backends have lowering code in place before introducing a 2x performance hit.

I agree with both.
Knowing that this is often a perf-critical pattern, we've put a lot of SDAG effort into optimization of it for x86 already, but it's a tricky problem because we get into a similar situation that we have with FMA. Ie, should we favor latency, throughput, or some combo?
Here's an example to show that we are trying to make the right decision per-CPU in codegen:
https://godbolt.org/z/s5GPqc
Note that the codegen choice can differ between scalar/vector - example in the X86.td file:

// FeatureFastScalarFSQRT should be enabled if scalar FSQRT has shorter latency
// than the corresponding NR code. FeatureFastVectorFSQRT should be enabled if
// vector FSQRT has higher throughput than the corresponding NR code.
// The idea is that throughput bound code is likely to be vectorized, so for
// vectorized code we should care about the throughput of SQRT operations.
// But if the code is scalar that probably means that the code has some kind of
// dependency and we should care more about reducing the latency.

Agreed "FeatureFastScalarFSQRT" can be removed if target thinks scalar FSQRT is costly. I see currently set at "SKXTuning" (Skylake).

In D85709#2210042, @cameron.mcinally wrote:

In D85709#2210034, @lebedev.ri wrote:

In D85709#2209979, @cameron.mcinally wrote:

I'm fairly sure this transform is a performance loss. For a target like Skylake Server, a SQRT(x) can take up to 20 cycles. But a RSQRT(x) is about 6 cycles and a MUL(y) is 4 cycles. We'd be better off with a X*RSQRT(X).

That is up to backends to decide. InstSimplify/InstCombine (and a few others) are canonicalization, target-independent passes.
A single sqrt(x) is more canonical IR than x/sqrt(x), because it's less instructions and x has less uses.

I agree with that. It should be canonicalized. It would also be good to make sure that the backends have lowering code in place before introducing a 2x performance hit.

Fixed test case to check for proper return value.

Looks Ok to me.

I ran SPEC2017 on Ryzen (c/c++ benchmarks) -O2 -fno-unroll-loops. no significant change in performance with the patch.

I agree removing the lengthy (9 byte) instructions and reducing size of the loop is good. But on performance side, I need to do some tests.

In D41723#977286, @chandlerc wrote:

In D41723#976780, @venkataramanan.kumar.llvm wrote:

In D41723#976776, @jyknight wrote:

Per kernel https://marc.info/?l=linux-kernel&m=151580566622935&w=2 and gcc https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01059.html, it seems AMD needs there to be an lfence in the speculation trap (and the pause is not useful for them, but does no harm). There seems to be some speculation (but no confirmation yet?) that pause *is* necessary vs lfence on intel. So in order to work generically, they seem to be suggesting using both instructions:

loop:
  pause
  lfence
  jmp loop

Some more links
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01209.html
and final patch:
https://github.com/gcc-mirror/gcc/commit/a31e654fa107be968b802786d747e962c2fcdb2b

In D41723#976776, @jyknight wrote:

Per kernel https://marc.info/?l=linux-kernel&m=151580566622935&w=2 and gcc https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01059.html, it seems AMD needs there to be an lfence in the speculation trap (and the pause is not useful for them, but does no harm). There seems to be some speculation (but no confirmation yet?) that pause *is* necessary vs lfence on intel. So in order to work generically, they seem to be suggesting using both instructions:

loop:
  pause
  lfence
  jmp loop

Some more links
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01209.html
and final patch:
https://github.com/gcc-mirror/gcc/commit/a31e654fa107be968b802786d747e962c2fcdb2b

Yes for AMD, we require "lfence" instruction after the "pause" in the "retpoline" loop filler. This solution has already been accepted in GCC and Linux kernel.
Can you please do the same in LLVM as well?

Ahh, I see my email crossed yours, sorry.

Have you tested adding 'lfence' and this patch on any AMD platforms? Do you have any results? Can you confirm that these patches are actually working?

In D41723#976776, @jyknight wrote:

Per kernel https://marc.info/?l=linux-kernel&m=151580566622935&w=2 and gcc https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01059.html, it seems AMD needs there to be an lfence in the speculation trap (and the pause is not useful for them, but does no harm). There seems to be some speculation (but no confirmation yet?) that pause *is* necessary vs lfence on intel. So in order to work generically, they seem to be suggesting using both instructions:

loop:
  pause
  lfence
  jmp loop

Some more links
https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01209.html
and final patch:
https://github.com/gcc-mirror/gcc/commit/a31e654fa107be968b802786d747e962c2fcdb2b