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'?

Updated to incorporate review suggestions

I don't know if there were other reviewers who haven't commented on how you addressed their concerns, but this looks good to me.

Could you make this a standalone patch that only modifies the table without adding the FP_CONTRACT support? Then the other patch can just be refactored to use this.

In D72425#1811118, @anemet wrote:

As Francis mentioned it before it would be good derive the pass name from the remark type (diag::remark_gvn_load_elim -> gvn) . I.e. I would drop the DEBUG_TYPE argument.

In D72425#1811098, @hfinkel wrote:

I like this general idea; couple of thoughts...

1. Clang has a similar system, and one disadvantage is that any time you change/add any message, it seems to trigger a large rebuild. Could we have this TG system generate separate .inc files for each category of things, so we don't have the same kind of rebuild problem.

lgtm

In D71635#1790611, @MaskRay wrote:

Before:

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: Support for floating point control option frounding-math is incomplete and experimental [-Wexperimental-float-control]

CC1 will do rounding math things.

After

% clang -frounding-math -fsyntax-only -x c /dev/null
clang-10: warning: optimization flag '-frounding-math' is not supported [-Wignored-optimization-argument]

CC1 will not do rounding math things. -fexperimental-rounding-math if the user really wants to use the feature.

Is my understanding correct? If yes, this patch seems pretty reasonable to me, because -frounding-math is currently incomplete/unsafe.

You may consider not changing CC1 options as they are not user facing.

In D62731#1788838, @rupprecht wrote:

It seems the discussion of whether or not this is incomplete died out -- I'd prefer to assume it is incomplete if there is no consensus. Mailed D71635 to rename -frounding-math to -fexperimental-rounding-math.

Alternatively we could remove the warning. I still don't see a good argument for the middle ground of having it called -frounding-math but also generate a warning.

I'm late jumping into the discussion of these patches and may not have seen all comments, so go easy on me if I say something that has already been covered.

In D62731#1784225, @rjmccall wrote:

... The big problem is that intrinsic calls are not arbitrary code: the vast majority of intrinsics (e.g. all the ARM vector intrinsics, many of which can be floating-point) are marked IntrNoMem (which I believe corresponds to readnone), which means calls to those intrinsics can be reordered with other code whether or not that code has arbitrary side-effects.

In D62731#1782912, @rjmccall wrote:

Hmm. The target-specific intrinsics thing does concern me, since I assume many targets have a bunch of vector intrinsics that may be sensitive to rounding. Do we have an idea of how we'd fix that? If it's a short-term incorrectness that we have a plan to fix, I don't mind the risk, but if we don't know how we'd even start to address it...

In D62731#1782762, @rjmccall wrote:

Currently we emit a warning if you use -frounding-math, saying that the option is ignored. That should have alerted users that they're not getting the correct behavior now. This patch removes the warning and (IIUC) implements the correct behavior but is over-conservative. If that's correct, I think that's acceptable and we don't need an "experimental" flag or a warning.

In D59780#1778882, @tstellar wrote:

In D59780#1778863, @ruiu wrote:

Hmm, I'm sorry but I'm confused. IIRC I had a discussion in the LLVM dev meeting that we were going to submit a change with a single PLT scheme rather than IPLT, so when I said that I'm going to submit a patch, I meant that I'm going to submit a patch for the 1PLT scheme rather than the 2PLT scheme. But this is for the 2PLT scheme. This is not something I want.

What was the decision that was made at the developer meeting? Will lld support the 2PLT scheme defined in the psABI?

I'm afraid we may have made a mess of this for X86 in D70214. We noticed the problem that you're fixing here, but I didn't realize you had a solution for it. I guess I hadn't looked at this patch. I'm sorry about that. Your solution looks good to me.

This was fixed in a different way.

Can this patch be abandoned?

I think we've agreed that this isn't a good use of tokens. The operand bundle approach may still be useful, but that can be attempted in a new revision.

To clarify, I was suggesting a single intrinsic with quiet/signaling encoded in the predicate. I think the IR definition is a bit cleaner this way and it maps to both the IEEE specification and at least the most common way (I think) this is implemented in x86 hardware and probably others. That said, if doing it this way makes the implementation more complicated I could go along with separate intrinsics and ISD opcodes.

I would prefer a single intrinsic/opcode with additional predicates to handle signalling vs. quiet. My main reason is consistency with how the IEEE-754 spec describes comparisons, but that's a weak argument. @craig.topper mentioned to me that there are some additional opcodes that might need to be duplicated if you treat signalling and quiet as separate operations.

lgtm

lgtm

As referenced in Simon's reply, I have started a mailing list discussion about this.

This looks good to me. I like the header file name and location you have here.