[PATCH] D114564: Fix the use of -fno-approx-func along with -Ofast or -ffast-math

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added a comment.

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.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2760
     case options::OPT_fno_honor_nans:       HonorNaNs = false;        break;
     case options::OPT_fapprox_func:         ApproxFunc = true;        break;
     case options::OPT_fno_approx_func:      ApproxFunc = false;       break;
----------------
Should this also imply "MathErrno = false"?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2762
     case options::OPT_fno_approx_func:      ApproxFunc = false;       break;
     case options::OPT_fmath_errno:          MathErrno = true;         break;
     case options::OPT_fno_math_errno:       MathErrno = false;        break;
----------------
Should this conflict with -fapprox-func?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2880
       break;
     case options::OPT_fno_unsafe_math_optimizations:
       AssociativeMath = false;
----------------
I think you need "AppoxFunc = false" here.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2937
       // subsequent options conflict then emit warning diagnostic.
       if (HonorINFs && HonorNaNs && !AssociativeMath && !ReciprocalMath &&
           SignedZeros && TrappingMath && RoundingFPMath &&
----------------
You need a check for "!ApproxFunc" here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114564