lgtm

This looks good. I have just a couple of minor nits.

@sepavloff I apologize for having lost track of this for so long. Do you have time to rebase this and the dependent patch?

I had forgotten that this patch never landed, but I was investigating a bug yesterday that I think this will help with (https://github.com/llvm/llvm-project/issues/48669).

This looks good to me. Thanks for the patch!

This example illustrates the problem this patch intends to fix: https://godbolt.org/z/j445sxPMc

The fix for the eval_method crash should be moved to a separate patch. Otherwise, this looks good. I have only minor comments.

In D120395#3359255, @pengfei wrote:

I don't agree. Unlike __fp16, __bf16 is simple an ARM specific type.

In D120395#3358533, @craig.topper wrote:

m256bh should not have been a new type. It should have been an alias of m256i. We don't have load/store intrinsics for m256bh so if you can even get the m256bh type in and out of memory using load/store intrinsics, it is only because we allow lax vector conversion by default. -fno-lax-vector-conversions will probably break any code trying to load/store it using a load/store intrinsic. If __m256bh was made a struct as at one point proposed, this would have been broken.

If we want m256bh to be a unique type using bf16, we must define load, store, and cast intrinsics for it. We would probably want insert/extract element intrinsics as well.

In D120395#3356355, @pengfei wrote:

Good question! This is actually the scope of ABI. Unfortunately, we don't have the BF16 ABI at the present. We can't assume what are the physical registers the arguments been passed and returned before we have such a hardware. For example, ARM has soft FP ABI that supports FP arithmetic operations and passes and returns arguments by integer registers. When we enabling some ISA set whose type doesn't have ABI representation, e.g., F16C, we borrowed such conception. And as a trade off, we used integer rather than introducing a new IR type, since we don't need to support the arithmetic operations.

In D120395#3346591, @scanon wrote:

There's a lot of churn around proposed "solutions" on this and related PR, but not a very clear analysis of what the problem we're trying to solve is.

Replacing __m128bh with __m128i does not prevent arithmetic operations on the type.

In D120395#3344890, @pengfei wrote:

Disscussed with GCC folks. We think it's better to use the same way as D120411 that replacing it with short int.

In D120395#3340953, @craig.topper wrote:

These intrinsics pre-date the existence of the bfloat type in LLVM. To use bfloat we have to make __bf16 a legal type in C. This means we need to support loads, stores, and arguments of that type. I think that would create bunch of backend complexity because we don't have could 16-bit load/store support to XMM registers. I think we only have load that inserts into a specific element. It's doable, but I'm not sure what we gain from it.

In D120395#3340496, @pengfei wrote:

Update LangRef. We use i16 type to represent bfloat16.

lgtm

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 ).