In D96646#2611589, @Tyker wrote:

In D96646#2594843, @andrew.w.kaylor wrote:

I'm not trying to be difficult, but I genuinely still don't understand the additional arguments pointer. Is it intended to allow proprietary extensions? Is there an example somewhere?

If these intrinsics are meant as a general mechanism to enable arbitrary communication between custom front ends and custom optimization passes, that's fine. I'd just like to see something explicitly explaining that.

the pointer is to a global struct containing the the provided arguments. I use it to communicate from source code to custom passes. but the front-end doesn't need to be custom. since the code to generate theses extra arguments is in clang trunk.
here is a basic example: https://godbolt.org/z/Tavcv9.
how the passes use this information is not defined it can be used for many things.
the major benefit I see is that it can pass arbitrary information and works well with template and constexpr programing. but the front-end does not do any semantic checking on the meaning of the information.

Thanks for the patch! This looks mostly good. I have just a few suggestions.

In D112760#3101260, @arsenm wrote:

This attribute has never been handled consistently in the backend. We have attributes approximately corresponding to each of the individual fast math flags, so the mirror of the IR controls would be to just set all of those attributes. I've never been sure how unsafe-fp-math fits in here, other than just a convenience to avoid setting all the others. I would also like to get rid of it, since you should be checking the individual properties anyway.

In D112760#3097718, @arsenm wrote:

I would rephrase the description as removing the global flag for contraction

Instead of generalizing the test, I re-ordered the checks to match what's currently happening. It appears to be stable and the test was broken by an explicit change to the object symbol ordering.

In D105895#2874128, @kpn wrote:

This patch may be overkill. If the reading of the floating point status register is restricted to "fpexcept.strict" then perhaps we don't need all of this code. But if we want to allow checking the status register after "fpexcept.maytrap" instructions then we do need all this. The LangRef restricts those reads to "fpexcept.strict". Are we certain we want to stick with that language?

FWIW, fp-contract=on has been the documented default for clang since version 5.

lgtm

In D104551#2909024, @kparzysz wrote:

Yes, that's exactly what happens. The project uses LLVM as a code generator for multiple targets. The OptBisector object is constructed the first time it's queried by shouldRunPass, which happened during the code generation for the first target. We set up the second target by passing some extra flags to ParseCommandLineOptions, but passing -opt-bisect-limit at this time no longer has any effect (i.e. the setting from the construction time remains).

Sorry for the non-response, I hadn't notice this review earlier.

lgtm

I think having a more robust library for FP comparisons in the test suite would be great. In fact, I thought about moving even the simple comparison I did in this patch to some common location where it could be included by other tests. Maybe that's a good starting point for an incremental change in the direction you're suggesting. A robust library of such checks is beyond the scope of what I have time to do at the moment, but I would certainly support that direction.

This seems OK as a short term solution, but it is also problematic in that it prevents FMA from being used in these performance measurements. I understand that it's not trivial to allow FP-related error tolerance in the test, but (as a future patch) it would be nice to at least have a way to turn off the exact result checking on FP tests so that the performance could be measured with fast-math and fp-contract enabled, and that would require having a way to re-enable FMA for these tests.

In D100091#2707536, @LiuChen3 wrote:

What is the usage model you’re trying to enable? This has been broken for a long time. Why is it important now?

I don't know any specific case for now. This was found by @andrew.w.kaylor. I will check with him.

In D99675#2671924, @efriedma wrote:

The expression “llvm.arith.fence(a * b) + c” means that “a * b” must happen before “+ c” and FMA guarantees that, but to prevent later optimizations from unpacking the FMA the correct transformation needs to be:

llvm.arith.fence(a * b) + c → llvm.arith.fence(FMA(a, b, c))

Does this actually block later transforms from unpacking the FMA? Maybe if the FMA isn't marked "fast"...

I'm not trying to be difficult, but I genuinely still don't understand the additional arguments pointer. Is it intended to allow proprietary extensions? Is there an example somewhere?

Addressed review feedback

In D95993#2579624, @fhahn wrote:

I just checked, and it appears the langref still only documents the 4 argument variants (https://llvm.org/docs/LangRef.html#llvm-ptr-annotation-intrinsic). I think those need to be updated as well?

Ping

Removed unnecessary bitcast

@pcc CODE_OWNERS.txt says that you own the bitcode. Can you help with this review?

lgtm

Sorry for having let this drop for so long. Some other priorities came up, but I am still interested in seeing this through.

In D79760#2033081, @pengfei wrote:

@andrew.w.kaylor I tested for below 3 cases, and get the runtime result for them in different compilers.

I don't understand why this difference exists. Beyond just trying to reproduce what MSVC does, can you explain the difference?

In D27028#1997092, @GGanesh wrote:

@andrew.w.kaylor I went through the mailing list thread regarding this change and saw "Eventually, we’ll want to go back and teach specific optimizations to understand the intrinsics so that where possible optimizations can be performed in a manner consistent with dynamic rounding modes and strict exception handling.".
Do you have any references\plans on how to teach specific optimizations on this?

Sorry for the delay in approval.

In D72675#1920309, @lebedev.ri wrote:

I may be wrong, but i suspect those failures aren't actually due to the fact
that we pessimize optimizations with this change, but that the whole execution
just fails. Can you try running test-suite locally? Do tests themselves actually pass,
ignoring the question of their performance?

OK. Since the behavior for out-of-range evl is target-dependent, undefined makes sense.

Can you use the "Edit Related Revisions" link to set the parent/child relationships of these patches and put "[1/5]" in the titles?

What is the intention of this set of patches in relation to D75938? It was unclear to me whether you intended to commit this implementation or were just offering it as an alternative for discussion.

I'm satisfied with the functionality, but I'm not sure about the intrinsics having undefined behavior outside the [0, W] range. The way you've implemented it, it seems like the behavior would be predictable. If the evl argument is outside that range, it is ignored. Applying an unsigned value greater than W using the "%mask AND %EVLmask" also has this effect. Why not just make that the defined behavior?

lgtm

I may have overstated. The exception behavior of the

Any more feedback? Should I proceed in this direction?

I don't know enough about PowerPC to review the check in the tests, but the basic constrained intrinsic handling looks correct.

It's a little weird that we're still recognizing these as LibFuncs, but I guess that's necessary to keep the optimizations like constant folding working with them. I see that ConstantFolding already calls the non-finite-restricted versions of the functions when it's doing compile-time evaluation. It would be good to have a comment somewhere (probably in TargetLibraryInfo.cpp below the note about math-finite.h) explaining the handling of these functions.

In D69891#1878624, @simoll wrote:

After I made that suggestion @craig.topper pointed out to me that the "experimental" qualifier has a tendency to never go away. See, for example, vector add/reduce intriniscs. All this is to say that my suggestion is just a suggestion and I could be convinced to drop it if that is the consensus.

How about we stick with llvm.vp.fadd and go for llvm.vp.fadd.v2, etc when/if the intrinsics are updated?

In D74436#1875315, @nemanjai wrote:

You're right, -O0 shouldn't generate FMA. I'm preparing to revert this now -- just verifying the build.

Perhaps this should be
off with no optimization
on with -O1/-O2/-O3/-Os/-Oz
fast with fast math

Just a suggestion, I'm not sure whether that would be the best breakdown. Perhaps we can also see what the defaults are for GCC and unify with those?

In D57504#1877268, @lkcl wrote:

In D57504#1877261, @andrew.w.kaylor wrote:

Perhaps this would be cleared up if I had a better understanding of what you were saying.

appreciated. if it's ok, can we schedule that for when it's part of a (new) proposal?

In D57504#1872493, @lkcl wrote:

In D57504#1872392, @craig.topper wrote:

MMX does use the X87 FP register file, but they can't coexist at the same. The first use of MMX marks the X87 register stack as occupied. I can't remember if it alters the data or not. An explicit emms instruction has to be done at the end of the MMX code to erase the MMX data and make the registers usable for X87 again.

craig, thank you for correcting me. that makes a lot of sense as i can just imagine the x87 designers going "argh, how are we going to avoid a pipeline clash / mess, here" :)

you get the principle i am sure, even though MMX is not a suitable example.

In D69891#1871485, @simoll wrote:

5. Rename to llvm.experimental.vp.* - Inserting experimental is the preferred way to introduce new intrinsics until they are stable (for technical reasons brought up by @chandlerc - https://reviews.llvm.org/D69891#1852795 ).

In D74436#1875320, @mibintc wrote:

However you are right we don't want the frontend to create FMA when optimizations are disabled -O0

In D74436#1875386, @thakis wrote:

The revert of this breaks tests everywhere, as far as I can tell.

lgtm

lgtm

In D57504#1861256, @simoll wrote:

Basically, I want to actively discourage front ends and optimizations from using the %evl argument in cases where it won't be optimal.

TTI would tell front ends and optimizations that %evl is a no-go for your target. Is this enough discouragement?

I feel like the bar should be fairly low for committing this. The proposal itself will notify those who are interested (agree or disagree) where they can join the discussion.

ping

OK. I was picturing MVL as some sort of maximum supported by the hardware in some sense or context. I think(?) I've got it now.

I like your first proposal for optional parameters. It "feels right" to me, and I agree that it is an improvement on operand bundles. Obviously it would take some time to build consensus for something like that as a new IR construct. I can live with the explicit parameter for now if we can agree on rules for how it is defined to behave on targets where it does not apply.

In D57504#1854330, @simoll wrote:

Exactly. The VE target strictly requires VL <= MVL or you'll get a hardware exception. Enforcing strict UB here means VP-users have to explicitly drop instructions that keep the VL within bounds. This means that we can optimize the VL computation code and that it can be factored into cost calculations, etc. With Options 2 & 3 this would happen only very late in the backend when most scalar optimizations are already done.

In D69891#1854113, @SjoerdMeijer wrote:

I'm not entirely clear where this value comes from, but it seems like whatever generated it must know that we're generating SVE code, right?

This is not architecture specific, and thus this is not assuming SVE. In the case of SVE, the vector length is unknown at compile time (it is a multiple of something), and constant at run-time. In the RISC-V vector extension, I believe it can be changed at any point. Thus, the way to obtain this value is by reading/writing a status register, or something similar, but relying on querying the architecture features. And whether it is constant at runtime, or can be changed at any point, this API seems to cover the different cases.

I know it's very late for me to be bringing this up, but I have a couple of broad questions -- one minor, one less minor.

In D57504#1851864, @cameron.mcinally wrote:

Btw, I guess that https://reviews.llvm.org/D71432 shows that op+select folding can be cleanly implemented in isel and that's also in line with my experiments for the VE target.

This needs a caveat. Keeping the select glued to the operation takes some careful effort. Especially in the undef passthru case, there are a bunch of peeps that will incorrectly fold away the select. E.g. this transform from InstSimplify:

if (isa<UndefValue>(FalseVal))   // select ?, X, undef -> X
  return TrueVal;

The VP intrinsics will certainly be immune to these, but if the plan is to eventually replace the VP select intrinsics with IR selects, then this problem will need to be solved. Just a heads up...

lgtm

Thanks for the review!

Rebase and fix typo

I have a couple of comments, but nothing that couldn't be addressed in a later patch.

It's not clear to me from reading this how the "precise" control is going to work with relation to the fast math flags. I don't think MSVC allows the granularity of control over these flags that we have in clang, so maybe they don't have this problem.

In D72675#1839492, @wristow wrote:

1. Should we enable FMA "by default" at (for example) '-O2'?