[SYCL][FE][Driver] Implement floating point accuracy control #8280
Conversation
This is a draft patch mostly to check that the use of TargetLibraryInfo can be used from the FE. Andy let me know if that's what you were thingking about?
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
| // on the function (IntrinsicID) and the command line accuracy (FPAccuracyIntrinsicID) | ||
| // FPAccuracyVal = TLI.getFPAccuracy(F->getName(), FPAccuracyIntrinsicID) | ||
| // auto *AccuracyMDS = MDString::get(CGF.Builder.getContext(), FPAccuracyVal); | ||
| return CGF.Builder.CreateCall(F, {Src0, AccuracyMD}); |
There was a problem hiding this comment.
I know the mock-up I shared used a metadata argument with a string like you have hear, but I ended up switching that to represent the accuracy as a numeric value using a callsite attribute. See PR #8134.
There was a problem hiding this comment.
Yes, I knew that was temporary. I was shooting for this IR:
call float @llvm.experimental.fpaccuracy.cos.f32(float %0, "float ulp-value")
but we need to have a way to calculate the ulp error. From you message in Team and comments below, it looks like the FE needs to compute this ulp error without going through the TargetLibraryInfoImp? So, it would be a new function depending on the library function name and the fp-accuracy option given in the command line (or FPA_default if no option is used)?
Do you mean fpbuiltin-max-error? This attribute still needs the ulp error value.
So, this is the IR you are expecting?
call float @llvm.experimental.fpaccuracy.cos.f32(float %0) #10
attributes #0 = { "fpbuiltin-max-error"="2.5" }
If yes, the FE needs to generate a new attribute to mark the function and calculate the ulp error value (the way I describe above).
There was a problem hiding this comment.
I think the mapping still needs to come from the backend because we'll want other languages (like Fortran) to be able to use the same functionality. I'll think about this and try to come up with a proposal. If you want to put a placeholder function in the front end for now, that's fine.
clang/lib/CodeGen/CGBuiltin.cpp
Outdated
| if (FPAccuracyIntrinsicID != Intrinsic::not_intrinsic) { | ||
| LangOptions::FPAccuracyKind FPAccuracy = CGF.getLangOpts().getFPAccuracy(); | ||
| // Enter this part of the condition only when TLI.isFPACCuracyAvailable is | ||
| // true, implying FPAccuary != LangOptions::FPA_Default. |
There was a problem hiding this comment.
I don't think this assumption is correct. It's possible that we'll have a back-end that supports multiple accuracy implementations but we want to use the default accuracy anyway. In fact, that's probably going to be the most common case.
…max-error attribute.
Added a map to LangOption that will map the function in the function list of command line to its accuracy.
| def warn_function_fp_accuray_already_set : Warning <"FP accuracy value of '%0' has already " | ||
| "been assigned to 8000 function '%1'">; | ||
| def warn_all_fp_accuray_already_set : Warning <"FP accuracy value of '%0' has already " | ||
| "been assigned to all functions in the program">; |
There was a problem hiding this comment.
Should there be a way to disable these diagnostics?
There was a problem hiding this comment.
Why would we want to disable them?
There was a problem hiding this comment.
It is an informative diagnostic, but typically warnings have a way to be disabled.
There was a problem hiding this comment.
I can create the warning in a new group:
def FPAccuracyWrongValue : DiagGroup<"wrong-value-of-fp-accuracy">;
and then use the -Wno-wrong-value-of-fp-accuracy on the command line to disable the warning. Is that what you mean?
There was a problem hiding this comment.
Yup, that's what I mean. Maybe: "fp-accuracy-value" would be sufficient here for the name.
|
@mdtoguchi Thanks for taking a look at it. |
| llvm::AttributeList AttrList; | ||
| // sincos() doesn't return a value, but it still has a type associated with | ||
| // it that corresponds to the operand type. | ||
| CGF.CGM.getFPAccuracyFuncAttributes( |
There was a problem hiding this comment.
Why do we need a AttrList here? Is there a scenario where more than one such attribute can be attached to a builtin call?
Thanks
There was a problem hiding this comment.
But there is a call to getFPAccuracyFuncAttributes which in tun will call getDefaultFunctionFPAccuracyAttributes that can additional attributes to the function. So. I think a list is needed here.
There was a problem hiding this comment.
Actually I think you are right that the AttrList can be a single attribute instead of a list. Added a TODO comment as we discussed offline. Thanks.
|
@intel/llvm-gatekeepers can you consider merging this please? Thanks. |
@zahiraam please update the caption of PR in accordance with https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/ContributeToDPCPP.md#commit-message |
|
Will be merged as soon as post-commit turns green. |
|
Failures in post-commit: https://github.com/intel/llvm/actions/runs/5267586087/jobs/9523035374 @zahiraam could you please address them? |
) This patch implements the accuracy controls for floating-point math functions in DPC++. Using the -ffp-accuracy command line option, the user can request an accuracy level for all math functions or for specific ones. Calls to fpbuiltin intrinsics llvm.fpbuilin.* are then generated. Syntax: Linux: -ffp-accuracy=[default|value][:funclist] Windows: /Qfp-accuracy:[default|value][:funclist] funclist is an optional comma separated list of math library functions. -ffp-accuracy=[default|value] default: Use the implementation defined accuracy for all math library functions. This is equivalent to not using this option. value: Use the defined standard accuracy for what each accuracy value means for all math library functions. -ffp-accuracy=[default|value][:funclist] default: Use the implementation defined accuracy for the math library functions in funclist. This is equivalent to not using this option. value: Use the defined standard accuracy for what each accuracy value means for the math library functions in funclist. value is one of the following values denoting the library function accuracy. high This is equivalent to max-error = 1.0. medium This is equivalent to max-error = 4. low This is equivalent to accuracy-bits = 11 for single-precision functions. accuracy-bits = 26 for double-precision functions. sycl Determined by the OpenCL specification for math function accuracy: https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_C.html#relative-error-as-ulps cuda Determined by standard https://docs.nvidia.com/cuda/cuda-c-programming-guide/#mathematical-functions-appendix
Any updates on this? If the fix isn't ready, can we just disable the tests to fix post-commit? |
@aelovikov-intel I was able to reproduce the issue and working on it. Yes for now, we can diable the LIT tests and I will work on it. Thanks. |
|
Also it looks like it's failing only on a release self-build. With the debug self-build the 2 tests are passing. |
This patch implements the accuracy controls for floating-point math functions in DPC++. Using the -ffp-accuracy command line option, the user can request an accuracy level for all math functions or for specific ones. Calls to fpbuiltin intrinsics llvm.fpbuilin.* are then generated.
Syntax:
Linux: -ffp-accuracy=[default|value][:funclist]
Windows: /Qfp-accuracy:[default|value][:funclist]
funclist is an optional comma separated list of math library functions.
-ffp-accuracy=[default|value]
default: Use the implementation defined accuracy for all math library functions.
This is equivalent to not using this option.
value: Use the defined standard accuracy for what each accuracy value
means for all math library functions.
-ffp-accuracy=[default|value][:funclist]
default: Use the implementation defined accuracy for the math library functions in funclist.
This is equivalent to not using this option.
value: Use the defined standard accuracy for what each accuracy value
means for the math library functions in funclist.
value is one of the following values denoting the library function accuracy.
high This is equivalent to max-error = 1.0.
medium This is equivalent to max-error = 4.
low This is equivalent to accuracy-bits = 11 for single-precision functions.
accuracy-bits = 26 for double-precision functions.
sycl Determined by the OpenCL specification for math function accuracy: https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_C.html#relative-error-as-ulps
cuda Determined by standard https://docs.nvidia.com/cuda/cuda-c-programming-guide/#mathematical-functions-appendix