This doesn't add metadata to llvm intrinsics that are not constrained.

In D93455#2465495, @simoll wrote:

In D93455#2463506, @kpn wrote:

In D93455#2462358, @simoll wrote:

In D93455#2460765, @kpn wrote:

Do you need tests for the verifier part?

Probably. With this patch standalone, we can only do "catch-22 testing": all uses of the bundle are illegal. We can extend that once the VP floating-point intrinsics are upstream that actually support these bundles.

Would replacing some of the constrained intrinsics with operand bundles on other intrinsics have the same safety guarantee in the middle end? Currently the middle end doesn't know about the constrained intrinsics so it handles code conservatively.

Not easily. This has the same re-occurring issue as enabling predication on all instructions: if we allow the constrained bundle on regular fp intrinsics overnight, existing optimization code will ignore the bundles and potentially break code.

That's a non-starter. People are relying on the strictfp options and #pragmas to work correctly already. That's why I had to add to clang the ability to turn it off the command-line option on targets that don't support it yet. So regressing and making strictfp unsafe again is just not an option.

An elegant way out for this would be to introduce an FPMathIntrinsic helper class that only matches intrinsics in the default fp environment (that is without the bundle). All code that looks for a specific fp math function can then no longer just take the intrinsic id but needs to cast the intrinsic to that helper class first.
Btw, the FPMathIntrinsic helper also fits well into the generalized pattern matching framework (D92086).

If there's a way to replace the constrained intrinsics with other, existing intrinsics plus operand bundles _while_ keeping the safety guarantee that we currently have then that sounds good. But the safety _must_ be the default until we get to updating optimizations to understand strictfp.

That's why i am proposing to initially use these bundles only for the VP intrinsics and keep the constrained fp intrinsics in place. When we are sure that all optimizations use the proper abstractions, we can move to replace constrained fp intrinsics with the bundled version instead.

In D93455#2462358, @simoll wrote:

In D93455#2460765, @kpn wrote:

Do you need tests for the verifier part?

Probably. With this patch standalone, we can only do "catch-22 testing": all uses of the bundle are illegal. We can extend that once the VP floating-point intrinsics are upstream that actually support these bundles.

Would replacing some of the constrained intrinsics with operand bundles on other intrinsics have the same safety guarantee in the middle end? Currently the middle end doesn't know about the constrained intrinsics so it handles code conservatively.

Not easily. This has the same re-occurring issue as enabling predication on all instructions: if we allow the constrained bundle on regular fp intrinsics overnight, existing optimization code will ignore the bundles and potentially break code.

Do you need tests for the verifier part?

Thanks for the quick turnaround!

Update for review comments.

No worries!

OK, I'll bite. I do know GOFF, having implemented it in a shipping, commercial compiler before. Give me some time to take a closer look.

LGTM

Update for review comments.

Update for review comments.

I'll see how much I can simplify of the tests.

Fix test, and eliminate a debugging "tee" command accidentally left in.

Add some tests for changing the rounding metadata.

I also added "-fexperimental-strict-floating-point" which makes it possible to write tests for a target when bringing up the support on that target. I think it would be confusing for that flag to work on command line arguments but not pragmas.

In D88913#2353379, @sepavloff wrote:

Generally the patch looks good. But the need to expect incorrect values in tests is a concern. Maybe this is a consequence of storing exception behavior in a separate field of CGFPOptionsRAII. This misbehavior should be fixed.

Ping

In D74729#2354279, @RKSimon wrote:

ok, I've no more questions @arsenm @kpn?

Fix a bug introduced in the last round, and eliminate the pass manager argument from one of the tests.

Updates from review.

Update/simplify for review comments.

Are you planning on supporting the case where an ED has a nonzero offset? For a general purpose dumper it matters: HLASM.

Unneeded since D85960 landed.

Say, in D80952 I added support for disabling strictfp support when a target doesn't support it. But it only applies to command line arguments.

In D87223#2265348, @qiucf wrote:

In D87223#2261086, @kpn wrote:

Over in D80952 there was a comment from @steven.zhang: "Agree that we can turn on it later when it is done." But your comment here is that it is "almost done".

Is it fair to say that the PowerPC work is at _parity_ with X86 and SystemZ? I think that's a better cutover point than "done".

Yes, we've disabled the FP mutation in rG6afb2791. vector-constrained-fp-intrinsics.ll is also aligned with X86 & SystemZ.

Over in D80952 there was a comment from @steven.zhang: "Agree that we can turn on it later when it is done." But your comment here is that it is "almost done".

It would be better to go with D85960. I'll hedge and keep this open until that one gets pushed.

You mentioned in D85920 a need to merge this review with that review. I don't think that's needed. This code here is farther along. It does everything that D85920 does and has necessary pieces implemented as well.

Thanks for the reviews and the fast turnaround! I do appreciate it!

Add the -fexperimental-strict-floating-point flag to enable on hosts that are not marked as supporting strict FP yet. Add test and documentation.

Already on it. I hope I got it now.

Ping. I'm really hoping to get this into 11. Otherwise we're going multiple releases with options that people already use causing crashes on most architectures.

Remove debugging command left in accidentally. Rebase.

Ping?

Ping.

Added new warnings to a new group "unsupported-floating-point-opt". The warning can be disabled. The disabling of constrained floating point is unaffected by the status of the warning.