Ah, errno. That was the missing piece. Got it.

Say, I can't find any file in the llvm repository named "GOFF*". They don't show up on Github, either, so I'm not sure what's going on. Where is the GOFF support in the LLVM tree currently? There's no "https://github.com/llvm/llvm-project/tree/main/llvm/lib/Object/GOFFObjectFile.cpp", for example.

Rebase after a precommit of the tests.

Phab just lost a comment of mine. Weird.

Update for review comments and reduce to just operating on NaN.

In D103169#2782902, @spatel wrote:

There's a lot going on here. I think we need to break this up to make sure we're testing the corner cases.

In D103169#2782902, @spatel wrote:

There's a lot going on here. I think we need to break this up to make sure we're testing the corner cases.

What if we start with plain constant folding? I don't see any existing tests for these intrinsics in llvm/test/Transforms/InstSimplify/ConstProp. See fp-undef.ll in that dir for test ideas.
For example, what can we do with this set of tests:

define float @fadd_undef_strict(float %x) #0 {
  %r = call float @llvm.experimental.constrained.fadd.f32(float undef, float undef, metadata !"round.dynamic", metadata !"fpexcept.strict") #0
  ret float %r
}

define float @fadd_undef_maytrap(float %x) #0 {
  %r = call float @llvm.experimental.constrained.fadd.f32(float undef, float undef, metadata !"round.dynamic", metadata !"fpexcept.maytrap") #0
  ret float %r
}

define float @fadd_undef_upward(float %x) #0 {
  %r = call float @llvm.experimental.constrained.fadd.f32(float undef, float undef, metadata !"round.upward", metadata !"fpexcept.ignore") #0
  ret float %r
}

define float @fadd_undef_defaultfp(float %x) #0 {
  %r = call float @llvm.experimental.constrained.fadd.f32(float undef, float undef, metadata !"round.tonearest", metadata !"fpexcept.ignore") #0
  ret float %r
}

What should happen if we replace each of those undef values with a constant number, NaN, etc?

Update for review comments.

In D99675#2696327, @pengfei wrote:

In D99675#2695424, @kpn wrote:

What changes are needed for a backend, and what happens if they aren't done?

As far as I understand it, backend does optimizations based on patterns of the known nodes and MIs. Inserting a new node/MI will block any optimizations across the fence. So it respects the semantics of the intrinsic without target special chenges.
I'm not sure if there's room for optimization cross the arithmetic.fence. If there is and no changes for it, backend may have some performance loss under these circumstances.

Having something needed for correctness silently not work seems ... sub-optimal.

I think backend is conservative for optimizations when use the intrinsic. It won't have correctness issue silently, but performance loss might.

We're still not ready to push this, but it can be used to find issues in clang. Once all tests pass with this change we can push it. We're not there yet.

Ping.

OK, I have nothing more. I'll leave formal signoff to @jhenderson.

I don't know the matrix implementation so I can't swear this hits every place needed, but the uses of CGFPOptionsRAII in this patch look correct at least.

There's still not much testing here, and there is plenty to test. The continuation records, for example. Tests can verify that all the invalid cases are rejected. Tests can also verify that GOFF is rejected if it is missing an HDR card or missing an END card. And since GOFF objects can be concatenated you can test that an object deck that is HDR+ESD+END+HDR+ESD+END is handled properly.

In D99675#2695480, @mibintc wrote:

In D99675#2695424, @kpn wrote:

What changes are needed for a backend, and what happens if they aren't done?

In the clang patch, I'm planning to add into TargetInfo a function like "does the target support __arithmetic_fence"?
In the llvm patch, the fallback implementation could be to merely ignore the call, and pass through the operand value. Is that adequate?

What changes are needed for a backend, and what happens if they aren't done?

In D100118#2695365, @kpn wrote:

I thought that adding a new fence required changing every optimization pass in LLVM. That's why the constrained intrinsics were implemented they way they are where no fence is needed.

Aren't you going to have miscompiles using this new fence until all that optimization work is done? Or am I wrong? @andrew.w.kaylor?

I thought that adding a new fence required changing every optimization pass in LLVM. That's why the constrained intrinsics were implemented they way they are where no fence is needed.

Address review comments: use early return.

The convention here is to wait a week or so before a ping, and the same between pings.

Test case?

You'll need to add much comprehensive testing like @jhenderson requested in the previous ticket.

In D89071#2610685, @jhenderson wrote:

The majority of your code is still untested as far as I can see. There appear to be three test cases you have so far:

1. An invalid size for a GOFF object.
2. A valid size for a GOFF object.
3. That getSymbolName returns the name of a single symbol in the symbol table.

What about all the rest of the functionality that is included in this patch, including, but certainly not limited to, the following?

1. More than one symbol in the symbol table.
2. Other properties of symbols.
3. The various properties of records.
4. Relocations.
5. And so on...

For each bit of code you have written, consider whether a test would fail if that bit of code was broken in some way, or didn't exist. If no test fails, then that code needs a new test case of some form. There may also be other cases where testing is appropriate, e.g. where two separate aspects of the same system interact in some way, although those are harder to judge.

System/Z's TEST DATA CLASS instruction covers most (all?) of the possible FP value states. You might want to subscribe, or add as a reviewer, jonpa just to make sure everyone stays in sync.

LGTM

In D95948#2542884, @mibintc wrote:

Should we add tests for mlong-double-64, -80, -128?

This looks like a definite step forward. Thank you!

Update for review comments: Move uses of CGFPOptionsRAII lower and closer to where they are needed. This should be less error prone as well.

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?