Page MenuHomePhabricator

Fix the use of -fno-approx-func along with -Ofast or -ffast-math
ClosedPublic

Authored by masoud.ataei on Nov 24 2021, 2:01 PM.

Diff Detail

Event Timeline

masoud.ataei requested review of this revision.Nov 24 2021, 2:01 PM
masoud.ataei created this revision.

Thanks for the patch! This looks mostly good. I have just a few suggestions.

Could you add test cases in clang/test/Driver/clang_f_opts.c to verify that the various driver inputs get overridden in the expected way? Without such a test, this behavior is likely to be broken in the future.

clang/lib/Driver/ToolChains/Clang.cpp
2760

Should this also imply "MathErrno = false"?

2762

Should this conflict with -fapprox-func?

2880

I think you need "AppoxFunc = false" here.

2938

You need a check for "!ApproxFunc" here.

masoud.ataei edited the summary of this revision. (Show Details)

Updated the test combining -ffast-math and -fno-approx-func options.
@andrew.w.kaylor I hope you don't mind that I put those tests on clang/test/Driver/fast-math.c instead.

masoud.ataei marked 2 inline comments as done.Nov 25 2021, 9:29 AM
masoud.ataei added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
2760

I don't think setting ApproxFunc to true should imply "MathErrno = false".

Let say someone have a math library that compute approximate result for none special input/output but returns NaN, INF and errno correctly otherwise. That is actually can be fairly common, because performance in the none special cases are much more important that the special ones. So returning errno in the special outputs theoretically should not effect the performance on the main path. Therefore, I think compiler should not assume anything about MathErrno value based on ApproxFunc value.

2762

Same as above.

Gentle reminder for reviewers. -- This PR is ready for review.

aaron.ballman edited reviewers, added: joerg, rjmccall; removed: aaron.ballman.Dec 7 2021, 8:25 AM

Removing myself as a reviewer because this is just far enough outside my expertise to feel uncomfortable reviewing it, but added a few trusted folks who have expressed opinions in this area before.

masoud.ataei added inline comments.Dec 9 2021, 11:41 AM
clang/lib/Driver/ToolChains/Clang.cpp
2760

I am not sure what I was suggesting in my last comment is correct or not. Can one of the more experienced reviewers confirm?
The question is: Should "ApproxFunc=ture" implies "MathErrno=false"?

joerg added inline comments.Dec 9 2021, 5:19 PM
clang/lib/Driver/ToolChains/Clang.cpp
2760

They seem pretty orthogonal to me.

A gentle reminder for reviewers. -- This patch will fix the bug reported here: https://bugs.llvm.org/show_bug.cgi?id=52565

andrew.w.kaylor accepted this revision.Tue, Jan 18, 10:46 AM

lgtm

clang/lib/Driver/ToolChains/Clang.cpp
2760

I see the point of your explanation. I'm satisfied that they should remain separate.

This revision is now accepted and ready to land.Tue, Jan 18, 10:46 AM
masoud.ataei closed this revision.Wed, Jan 19, 8:08 AM