[PATCH] D93455: Constrained fp OpBundles

Mon Dec 21 10:28:44 PST 2020

kpn added a comment.

In D93455#2465495 <https://reviews.llvm.org/D93455#2465495>, @simoll wrote:

> In D93455#2463506 <https://reviews.llvm.org/D93455#2463506>, @kpn wrote:
>
>> In D93455#2462358 <https://reviews.llvm.org/D93455#2462358>, @simoll wrote:
>>
>>> In D93455#2460765 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://llvm.org/docs/LangRef.html#operand-bundles>) . 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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 either the global metadata 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93455/new/

https://reviews.llvm.org/D93455