[PATCH] D138867: [RFC] Add new intrinsics and attribute to control accuracy of FP calls

Kevin P. Neal via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 13 11:54:24 PST 2022


kpn added inline comments.


================
Comment at: llvm/docs/LangRef.rst:22593
+intrinsic has any callsite attributes begining with "fp-" that that code
+performing the transformation does not recognize.
+
----------------
andrew.w.kaylor wrote:
> kpn wrote:
> > Is there any way to enforce this? If the constrained intrinsics are merged in with the non-constrained then we lose the safe-by-default property.
> > 
> > The paragraph above would be stronger if it said "must not" instead of using the word "should".
> The safe-by-default property only ever came from the fact that existing optimizations didn't recognize the intrinsics at all. Initially we'd get that same benefit with these new intrinsics.
> 
> I was thinking I could add a function like FPBuiltinIntrinsic::hasUnrecognizedFPAttrs() that would take a list of IDs of FP attributes that the caller did know about. Then as we teach a pass to use these intrinsics, we can also bake in the list of attributes that the pass knows how to check for. I'm imagining a pattern something like this:
> 
> ```
> RecognizedFPAttrs.push_back(FPBuiltinIntrinsic::FP_MAX_ERROR);
> if (FPI->hasUnrecognizedFPAttrs(RecognizedFPAttrs)
>   return false;
> if (FPI->getRequiredAccuracy() != None)
>   return false;
> // Do the transformation
> 
> ```
> How does that sound?
I'm a little worried that that much code would get "refactored" into a function out of sight of authors and reviewers and therefore encourage mistakes. Maybe it'll be OK.

It's going to require more vigilance watching for changes to be certain that nobody accidentally slips in a change that breaks the strictfp support. It's also going to be work to be sure that anyone making changes understands all of the different fp attributes. Cross-training is a good thing, but to be clear it is also a cost.  

Does it make sense to include builtins that replace LLVM instructions? Add, subtract, etc? Are there going to be different add implementations that have the different precision issues seen with library calls?

I admit I'm a little nervous. and I'm sorry I can't be more concrete about my concerns.

The IR matchers will have to be dealt with at some point. Granted, this is also true without your proposal.


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

https://reviews.llvm.org/D138867



More information about the llvm-commits mailing list