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.

Thanks. I understand your direction for denormal handling now, and I'm OK with this patch apart from the remaining references to subnormal that Sanjay mentioned.

In D70096#1741213, @kpn wrote:

Do we want to restrict this so it doesn't get upgraded if found in a future bitcode version?

@pcc I've added you as a reviewer in hopes that you can tell me whether my use of code in globalCleanup() is correct. It seems to be working, but Craig was concerned that there might be cases under which this would be called when things weren't sufficiently materialized to properly handle the distinction between function declaration and definition.

I'm unclear as to the expectations surrounding this option. I suppose this is somewhat beyond the scope of the current changes, but I'm confused by clang's current behavior with regard to denormals.

I like this. I have just one minor suggestion.

This looks like a very good start. I've talked to @pengfei about the x86 backend support for this. As long as x86 doesn't fail horribly, I'd be OK with the x86 work being done in a separate patch that depends on this one.

In D57504#1725554, @simoll wrote:

• 1. Observations
• When using fpexcept.ignore, the fp callsites should have the readnone attribute set on them to override the inaccessiblememonly of the intrinsic declarations. That way DCE still works.

Thanks for the patch! I don't have time to review this in detail this week, but I'm very happy to see this functionality.

This patch seems to be doing at least two different things. Can you separate the changes related to strict node handling into a separate patch?

This looks good to me, but I'd like @craig.topper to verfiy that he's happy with the parts he commented on.

I assume you've tested this with your verifier changes. This is a bit too much to verify every call and function manually. I looked at the ones that weren't boilerplate updates, and I sampled the boilerplate changes. Based on that review strategy, it looks good to me.

In D67839#1683038, @kpn wrote:

In D67839#1681121, @uweigand wrote:

I agree with marking function call sites as "strictfp".

Say, would this apply to all function call sites? Or just to ones that aren't intrinsics?

This looks good to me.

It was probably me that argued for the strictfp attribute being required on the function definition. My reasoning was that when the inliner wants to inline a function containing strict FP calls into a function that does not, it would use the strictfp attribute as a cue to convert all of the FP operations in the target function to constrained. This can, of course, be deduced from the IR, but if we treat the attribute on the definition as optional then the inliner would always need to check the IR anyway. So if it isn't required it has limited usefulness.

Need to fix Sphinx warnings

Thanks for updating this bit of documentation!

In D67360#1668007, @kpn wrote:

Address review comment. Mention the possibility of the restrictions mentioned here changing in the future, but don't make any promises.

It took some digging, but I finally found the e-mail thread where we initially agreed that we can't mix constrained FP intrinsics and non-constrained FP operations within a function. Here it is: http://lists.llvm.org/pipermail/cfe-dev/2017-August/055325.html

In D66092#1632642, @sepavloff wrote:

• What is the issue with moving a = b/c? If it moves ahead of if statement it seems OK, because the rounding mode is the same in that point. It cannot be moved inside the block (where rounding mode is different) because it breaks semantics.

In D66092#1630997, @sepavloff wrote:

Replacement of floating point operations with constrained intrinsics seems more an optimization helper then a semantic requirement. IR where constrained operations are mixed with unconstrained is still valid in sense of IR specification.

In D66092#1627339, @sepavloff wrote:

In D66092#1625380, @kpn wrote:

Also, if any constrained intrinsics are used in a function then the entire function needs to be constrained. Is this handled anywhere?

If we decided to make the entire function constrained, it should be done somewhere in IR transformations, because inlining may mix function bodies with different fp options.

In D62731#1623114, @mibintc wrote:

@andrew.w.kaylor Thanks Andy. Reminder -- in a private document you indicated to me that -fp-speculation=safe corresponds to the maytrap setting for the exception argument. This patch is implemented with those semantics.

I'm not entirely caught up on this review. I've only read the most recent comments, but I think I've got enough context to comment on the metadata arguments.

My understanding is that IEEE-754 requires us to produce an exception if and only if the exception would be produced by a literal interpretation of the source. However, it does not require that the exceptions be raised in the same order as implied by the source. Also, that's what the LLVM language reference says we'll do with "fpexcept.strict" -- "The number and order of floating-point exceptions is NOT guaranteed." So, I think the changes you've got here are correct.

In D64067#1568895, @hfinkel wrote:

One thing to realize about these flags is that they're ABI-altering flags. If the user provides the flag to alter the platform defaults, this only works if the user also ensures that matches the ABI of the relevant system libraries that the compiler is using (e.g., because the user is explicitly linking with a suitable libc).

In this review (https://reviews.llvm.org/D6260) @rsmith mentions that this should also have an effect on name mangling.

I might be opening a can of worms here and I'm not a language expert, but it isn't clear to me from reading the C99 standard that defining fptosi/fptoui as returning poison values in the unrepresentable case allows correct implementation of the C standard. That is, it doesn't seem to me that the standard actually says this is undefined behavior. It just says the resulting value is unspecified, and the exception behavior is explicitly defined. On the other hand, C++ does say clearly that it is undefined behavior, right?

For the constrained intrinsics, at least in the case where the exception semantics argument is "fpexcept.strict", we need to make sure the invalid exception is raised. If the exception semantics argument is "fpexcept.ignore" or "fpexcept.maytrap" we could allow the conversion to be optimized away.

In D43515#1548845, @kpn wrote:

I believe the speculative exceptions should be largely fixed by r348251, committed by rksimon. I changed the code to continue to use strict nodes when expanding a strict node. Since the strict nodes are chained, does that not solve the part of the problem not solved by r348251? Is the existing comment not enough, or do I need to copy some of the commit message into code comments?

How would you feel about rebooting this as a new patch? There's a lot of irrelevant history here, and I feel like I'm missing some context as I review it.

OK. I'm convinced. This should let us get a correct solution in place, and as you say we can add on something to handle rounding modes later if needed.

In D55506#1527992, @uweigand wrote:

In D55506#1527651, @andrew.w.kaylor wrote:

Why is the FPBarrierChain getting involved with the BarrierChain for memory objects? Even without this patch I think we might be entangling the strict FP nodes with other chained nodes more than we should.

The BarrierChain tracks calls, volatile/atomic memory accesses, and UnmodeledSideEffects. I do believe that FP exceptions must indeed be kept stable relative to those.

My patch does *not* entangle FP exceptions with regular memory accesses, which do *not* touch BarrierChain.

Why is the FPBarrierChain getting involved with the BarrierChain for memory objects? Even without this patch I think we might be entangling the strict FP nodes with other chained nodes more than we should.