This is an archive of the discontinued LLVM Phabricator instance.

[clang] Option control afn flag
ClosedPublic

Authored by masoud.ataei on Jul 16 2021, 2:14 PM.

Details

Summary

Proposing an clang option to control afn flag.

This change is proposed as a support for: https://reviews.llvm.org/D101759

Diff Detail

Event Timeline

masoud.ataei created this revision.Jul 16 2021, 2:14 PM
masoud.ataei requested review of this revision.Jul 16 2021, 2:14 PM

Remove extra function deceleration.

qiucf added a comment.Aug 26 2021, 3:03 AM

Making a new option mapped to another float op flag looks reasonable, but is there any clearer motivation for this? (such as the need for -Ofast -fno-approx-func)

llvm/include/llvm/Target/TargetOptions.h
179

-enable-no-signed-zeros-fp-math is an llc flag.

Do we really have -enable-approx-func-fp-math for llc now?

masoud.ataei added a comment.EditedAug 26 2021, 7:30 AM

Making a new option mapped to another float op flag looks reasonable, but is there any clearer motivation for this? (such as the need for -Ofast -fno-approx-func)

This patch https://reviews.llvm.org/D101759 is the real motivation for option controlling afn flag. We want to have a way to distinguishes getting _finite or non-finite version of MASS functions. Only with afn flag (on O3), we want to get non-finite version of MASS functions. (finite version need extra fast-math flags.)

llvm/include/llvm/Target/TargetOptions.h
179

I am adding and using this option in https://reviews.llvm.org/D101759 patch.

FWIW i think this part is fine in principle.
Not sure about errno.

bmahjour added inline comments.Aug 30 2021, 2:32 PM
clang/include/clang/Driver/Options.td
1732–1733

So this option already exists and seems to behave the way we want it to. Does anyone know why it was made NoDriverOption?

> cat fp.c
#include <math.h>
void foo(float *f1, float *f2)
{
  *f1 = sin(*f2) + *f1;
}
> clang -c fp.c -S -emit-llvm -mllvm -disable-llvm-optzns -O3 -Xclang -fapprox-func && grep afn fp.ll
  %call = call afn double @sin(double %conv) #2
  %add = fadd afn double %call, %conv1

Could we just expose it as a supported option and call it done. ie make it more like fhonor_nans below but without introducing a new function attribute:

def fapprox_func : Flag<["-"], "fapprox-func">, Group<f_Group>;

so that instead of having -Xclang -fapprox-func in the command above we could just have -fapprox-func ?

clang/test/CodeGen/afn-flag-test.c
10

can we avoid these attributes?

13

avoid attributes.

masoud.ataei marked 4 inline comments as done.Sep 28 2021, 9:41 AM

Sorry that it took me so long to reply reviews. Thank you for reviewing this patch.

clang/include/clang/Driver/Options.td
1732–1733

With the change that I proposed for approx_func option, we don't need to pass it with -Xclang. So, in your example, we can simply remove -Xclang and it will work the same:

> clang -c fp.c -S -emit-llvm -mllvm -disable-llvm-optzns -O3 -fapprox-func && grep afn fp.ll
  %call = call afn double @sin(double %conv) #2
  %add = fadd afn double %call, %conv1

(even no need for -mllvm -disable-llvm-optzns -O3)

Example for -fapprox-func and -fno-approx-func:

$ clang -c fp.c -S -emit-llvm -fapprox-func && grep afn fp.ll
  %call = call afn double @sin(double %conv) #2
  %add = fadd afn double %call, %conv1
$ clang -c fp.c -S -emit-llvm -fno-approx-func && grep afn fp.ll
$
clang/lib/CodeGen/CGCall.cpp
1818

Regarding the attributes discussion below, here is place that attributes are added.

clang/test/CodeGen/afn-flag-test.c
10

I understand that attributes are less desirable now in LLVM. But all other options for fast-math flags (-fhonor-nans, -fhonor_infinities, ...) add the attributes too. So, to be consistent with others I added this attribute too.

Example: This will add "no-nans-fp-math"="true" attribute along with nnan flag.

$ clang -c fp.c -S -emit-llvm -fno-honor-nans
13

Same as above.

masoud.ataei marked 3 inline comments as done.Sep 28 2021, 9:43 AM

Added @jansvoboda11 to the review as it appears he was the one that added the original option.

Reminder for reviewers.

lebedev.ri added subscribers: erichkeane, aaron.ballman.

Looks reasonable to me.
@aaron.ballman @erichkeane does it seem complete or are we missing some infra piece?

clang/include/clang/Driver/Options.td
1733

Missing description

Looks reasonable to me.
@aaron.ballman @erichkeane does it seem complete or are we missing some infra piece?

At least from the CFE's side I think this is complete as far as I can see. I agree we need to have the description. I would also like to see a Driver test on this as well.

Agreed on the request for description and driver test coverage. I'm also wondering if there should be a release note and documentation because it's lifting a private flag to be a public one. e.g., should we update https://clang.llvm.org/docs/UsersManual.html#controlling-floating-point-behavior ?

Description and driver test are added.

masoud.ataei marked an inline comment as done.Oct 7 2021, 10:32 AM

Update the documentation.

erichkeane added inline comments.Oct 7 2021, 11:03 AM
clang/docs/UsersManual.rst
1393 ↗(On Diff #377909)

This seems pretty incomplete to me, I have no idea what it does from this description. Can you elaborate?

masoud.ataei added inline comments.Oct 7 2021, 1:38 PM
clang/docs/UsersManual.rst
1393 ↗(On Diff #377909)

If I didn't miss to say it is about math function calls, then I guess it would be more clear.

Allow substitution of approximate calculations for math function calls.

This option just adds afn fast-math flag to the function calls and in backend the math function call can be replaced by an approximate calculation (either another math function call or not).
https://llvm.org/docs/LangRef.html#fast-math-flags

erichkeane added inline comments.Oct 7 2021, 2:16 PM
clang/docs/UsersManual.rst
1393 ↗(On Diff #377909)

allow substitution of approximate calculations for math function with what? I'm not clear as to what this does still. I'm not sure that I get what the 'afn' documentation means there either. Typically we'd want the clang frontend flags to be more accurate/descriptive than the LLVM-IR documentation.

masoud.ataei marked an inline comment as not done.Oct 8 2021, 8:26 AM
masoud.ataei added inline comments.
clang/docs/UsersManual.rst
1393 ↗(On Diff #377909)

This option adds afn fast-math flag to function calls in IR. And in the case of math function calls (like log, sqrt, pow, ...), afn flag allows LLVM to substitute the math calls with an "approximate calculation". What an approximate calculation is may differ based on which math call it is, what other fast-math flags are set, what is the target machine, and other factors.

An approximate calculation can be

  1. sequence of instructions (inlining): for example, inlining sqrt call needs afn flag to be present in the call.
  2. a substituted math function call (which may be less accurate but faster): this substitution can be (a) for general targets: for example: pow(x,0.25) to sqrt(sqrt(x)). or (b) for a specific target: for example, in the case of PowerPC, we are proposing to substitute math calls to MASS library calls https://reviews.llvm.org/D101759

Note that, afn flag is necessary for these approximate calculation but may not be sufficient. Most of them need at lease one or two other fast-math flags to be set too.

I agree the term "approximate calculation" is a very general term. But I guess it is needed to describe this general situation.

erichkeane added inline comments.Oct 8 2021, 8:34 AM
clang/docs/UsersManual.rst
1393 ↗(On Diff #377909)

Perhaps then something like:
Allow certain math function calls (such as log, sqrt, pow, etc) to be replaced with an approximately equivalent set of instructions or alternative math function calls. For example, a pow(x, 0.25) may be replaced with sqrt(sqrt(x), despite being an inexact result in cases where <expand a little>.

For Options.td we can be a little more terse, so perhaps something a little more like:

Allow certain math function calls to be replaced with an approximately equivalent calculation.

@aaron.ballman WDYT?

aaron.ballman added inline comments.Oct 8 2021, 8:43 AM
clang/docs/UsersManual.rst
1393 ↗(On Diff #377909)

I think the docs suggestion makes sense (though please be sure to add backticks to all the syntax components, correct the missing paren on sqrt(sqrt(x)), and finish the <expand a little> bit.

I think the suggested wording for Options.td looks good. Thanks!

Update the documentation.

masoud.ataei marked 4 inline comments as done.Oct 8 2021, 9:40 AM

1 small 'english' question, hopefully Aaron can answer, otherwise LGTM.

clang/docs/UsersManual.rst
1395 ↗(On Diff #378277)

Should this be 'alternate math function calls'? I'm second guessing myself now.

aaron.ballman accepted this revision.Oct 8 2021, 9:54 AM

LGTM

clang/docs/UsersManual.rst
1395 ↗(On Diff #378277)

alternative is correct. alternate as an adjective means "every other, every second".

This revision is now accepted and ready to land.Oct 8 2021, 9:54 AM
erichkeane accepted this revision.Oct 8 2021, 10:05 AM
masoud.ataei closed this revision.Oct 8 2021, 11:54 AM