This is an archive of the discontinued LLVM Phabricator instance.

[RFC] Add new intrinsics and attribute to control accuracy of FP calls
Needs ReviewPublic

Authored by andrew.w.kaylor on Nov 28 2022, 2:36 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

This patch adds a new set of fpbuiltin intrinsics to represent operations that are equivalent to common math library functions and basic operations, and adds a new call site attribute ("fp-max-error") to specify the required accuracy of these calls.

The purpose of these new IR constructs is to support alternate math library implementations and provide a general mechanism for selecting among multiple implementations based on specific requirements.

This is a follow-up to discussions here: https://discourse.llvm.org/t/rfc-floating-point-accuracy-control/66018

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 2:36 PM
andrew.w.kaylor requested review of this revision.Nov 28 2022, 2:36 PM
Matt added a subscriber: Matt.Nov 30 2022, 4:52 PM

Add Language Reference documentation for the new intrinsics and attribute.

kpn added a subscriber: kpn.Dec 5 2022, 10:59 AM
kpn added inline comments.
llvm/docs/LangRef.rst
22593

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

llvm/docs/LangRef.rst
22593

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?

kpn added inline comments.Dec 13 2022, 11:54 AM
llvm/docs/LangRef.rst
22593

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.

titeup added a subscriber: titeup.Dec 21 2022, 9:53 AM

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.

llvm/docs/LangRef.rst
22593

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.

Here is a link to the pull request in the SYCL development fork: https://github.com/intel/llvm/pull/8134