Page MenuHomePhabricator

Constrained fp OpBundles
Needs ReviewPublic

Authored by simoll on Dec 17 2020, 5:49 AM.

Details

Summary

This adds support for the cfp-except and cfp-round operand bundles. These bundles can be tagged on to floating-point intrinsics to specify a non-default floating environment.

The first application, we are preparing with this patch, is to support non-default fp environments in VP intrinsics (eg llvm.vp.fadd). However, constrained fp bundles could replace all redundant intrinsics that are just another intrinsic plus metadata params for the fp environment (eg llvm.experimental.constrained.sqrt).

This patch won't be committed before floating-point VP intrinsics are in (unless people would like to use these bundles right away for constrained fp, of course).

Diff Detail

Event Timeline

simoll created this revision.Dec 17 2020, 5:49 AM
simoll requested review of this revision.Dec 17 2020, 5:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2020, 5:49 AM
simoll updated this revision to Diff 312478.Dec 17 2020, 6:10 AM
  • NFC. Fixed spelling, typos.
simoll updated this revision to Diff 312479.Dec 17 2020, 6:12 AM

nfc, comment cleanup.

simoll updated this revision to Diff 312486.Dec 17 2020, 6:59 AM
  • Fixed a failing tests (there is now two more op bundles)
  • Rebased.
kpn added a comment.Dec 17 2020, 9:16 AM

Do you need tests for the verifier part?

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. What about intrinsics the middle end already knows about? Would the new operand bundle be as safe as the constrained intrinsics are now?

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

What about intrinsics the middle end already knows about?

Constrained bundles and the existing constrained intrinsics can exist side by side if all places that reason about constrained fp do so through the ConstrainedFPIntrinsic class. Once that is the case and we want to switch on bundles on some intrinsics, we only need to wire that up in ConstrainedFPIntrinsic .

Would the new operand bundle be as safe as the constrained intrinsics are now?

Yes, it is illegal to drop operand bundles (see the LangRef) . The bundle is just as stable as the regular operand list of the intrinsics. One thing to take care of is proper use of callsite attributes: when the bundle is used with non-default settings, the call site must have the strictfp attribute and potentially others depending on the fp configuration (eg it may trap, access memory (fp config registers), ..).

kpn added a comment.Dec 18 2020, 9:56 AM
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.

What about intrinsics the middle end already knows about?

Constrained bundles and the existing constrained intrinsics can exist side by side if all places that reason about constrained fp do so through the ConstrainedFPIntrinsic class. Once that is the case and we want to switch on bundles on some intrinsics, we only need to wire that up in ConstrainedFPIntrinsic .

I see no reason to keep duplicate intrinsics. Once other intrinsics are able to replace the constrained ones then we should drop the llvm.experimental.constrained.* set. We do need to make sure there's an easy way to check that an intrinsic call is one of these constrained-replacement intrinsics, if only for the IR Verifier, but hopefully that'll be straightforward and not error prone.

Would the new operand bundle be as safe as the constrained intrinsics are now?

Yes, it is illegal to drop operand bundles (see the LangRef) . The bundle is just as stable as the regular operand list of the intrinsics. One thing to take care of is proper use of callsite attributes: when the bundle is used with non-default settings, the call site must have the strictfp attribute and potentially others depending on the fp configuration (eg it may trap, access memory (fp config registers), ..).

I still have D68233 open, but I'm blocked because clang's C++ support isn't ready yet. And clang's handling of the AST isn't ready yet either because it drops metadata. Plus, I have the successor to D68233 still that verifies that non-strict and strictfp are never mixed in a function.

In D93455#2463506, @kpn 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.

What about intrinsics the middle end already knows about?

Constrained bundles and the existing constrained intrinsics can exist side by side if all places that reason about constrained fp do so through the ConstrainedFPIntrinsic class. Once that is the case and we want to switch on bundles on some intrinsics, we only need to wire that up in ConstrainedFPIntrinsic .

I see no reason to keep duplicate intrinsics. Once other intrinsics are able to replace the constrained ones then we should drop the llvm.experimental.constrained.* set. We do need to make sure there's an easy way to check that an intrinsic call is one of these constrained-replacement intrinsics, if only for the IR Verifier, but hopefully that'll be straightforward and not error prone.

If we can cast it to a ConstrainedFPIntrinsic, it is legal either because it is an existring intrinsic + bundle or an intrinsics of the llvm.experimental.constrained.* kind. And i agree, we should retire the constrained intrinsics as soon as it is safe to do so.

Would the new operand bundle be as safe as the constrained intrinsics are now?

Yes, it is illegal to drop operand bundles (see the LangRef) . The bundle is just as stable as the regular operand list of the intrinsics. One thing to take care of is proper use of callsite attributes: when the bundle is used with non-default settings, the call site must have the strictfp attribute and potentially others depending on the fp configuration (eg it may trap, access memory (fp config registers), ..).

I still have D68233 open, but I'm blocked because clang's C++ support isn't ready yet. And clang's handling of the AST isn't ready yet either because it drops metadata. Plus, I have the successor to D68233 still that verifies that non-strict and strictfp are never mixed in a function.

Wait. So you are saying clang drop the metadata arguments in constrained fp intrinsics?

kpn added a comment.EditedDec 21 2020, 10:28 AM
In D93455#2463506, @kpn 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.

OK, that sounds like a good plan. And clang uses the IRBuilder with its easy support for constrained floating point, so the number of places to touch is smaller than one might think. Mostly just CGBuiltins.cpp would need to be changed.

Would the new operand bundle be as safe as the constrained intrinsics are now?

Yes, it is illegal to drop operand bundles (see the LangRef) . The bundle is just as stable as the regular operand list of the intrinsics. One thing to take care of is proper use of callsite attributes: when the bundle is used with non-default settings, the call site must have the strictfp attribute and potentially others depending on the fp configuration (eg it may trap, access memory (fp config registers), ..).

I still have D68233 open, but I'm blocked because clang's C++ support isn't ready yet. And clang's handling of the AST isn't ready yet either because it drops metadata. Plus, I have the successor to D68233 still that verifies that non-strict and strictfp are never mixed in a function.

Wait. So you are saying clang drop the metadata arguments in constrained fp intrinsics?

It's not quite that simple. First, if clang is in strictfp mode for a function then it stays in strictfp mode for that function. So we'll still get constrained intrinsics. There's no issue there.

If the command line arguments are used to enable strictfp AND no #pragma is used to affect the compiler's idea of the FP environment then there's no problem. It will use the global metadata which will be in the AST so, again, no problem.

However, if strictfp is enabled on the command line and #pragma is used on the FP environment then we're running into places where the global metadata is present in the AST _instead_ _of_ the correct metadata based on the #pragma. You can see this in tests that set "maytrap" on the RUN line and then use a #pragma to go fully strict at the top of the test. Broken tests will have a mixture of strict and maytrap. Note that only a handful of tests have been changed to do that so far.

It's a work in progress to fix, and I still haven't figured out a good way to prevent regressions.