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

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added a comment.
Herald added a subscriber: luke.

This doesn't seem to have drawn a lot of support, perhaps because the use cases for the accuracy control are rather abstract right now. I'm going to move this over to the repository we use for SYCL development (https://github.com/intel/llvm) because I need to make progress with the implementation there. Hopefully once we have some open source libraries that leverage the interfaces I'm adding I'll be able to bring it back here with a bit of momentum behind it. I'll post a link here once I have a pull request up.

In the meantime, I hope we'll consider this as a model for any additional fp-related intrinsics we may think are needed.



================
Comment at: llvm/docs/LangRef.rst:22593
+intrinsic has any callsite attributes begining with "fp-" that that code
+performing the transformation does not recognize.
+
----------------
kpn wrote:
> 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.
Sorry for the long silence.

> 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?

The inclusion of the operations like add and subtract was in response to a request from an FPGA compiler developer I talked to. Apparently in the FPGA world there is a potential use case for less accurate add and subtract. I don't know how that works, but I'm taking his word for it.




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

https://reviews.llvm.org/D138867