masoud.ataei marked 4 inline comments as done. masoud.ataei added a comment.
Sorry that it took me so long to reply reviews. Thank you for reviewing this patch. ================ Comment at: clang/include/clang/Driver/Options.td:1732-1733 NegFlag<SetFalse>>; -def fapprox_func : Flag<["-"], "fapprox-func">, Group<f_Group>, Flags<[CC1Option, NoDriverOption]>, - MarshallingInfoFlag<LangOpts<"ApproxFunc">>, ImpliedByAnyOf<[menable_unsafe_fp_math.KeyPath]>; defm finite_math_only : BoolFOption<"finite-math-only", ---------------- bmahjour wrote: > 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 `? 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 $ ``` ================ Comment at: clang/lib/CodeGen/CGCall.cpp:1818 + if (LangOpts.ApproxFunc) + FuncAttrs.addAttribute("approx-func-fp-math", "true"); if (LangOpts.UnsafeFPMath) ---------------- Regarding the attributes discussion below, here is place that attributes are added. ================ Comment at: clang/test/CodeGen/afn-flag-test.c:10 + // CHECK-AFN: %{{.*}} = call afn double @{{.*}}exp{{.*}}(double %{{.*}}) + // CHECK-AFN: attributes #0 ={{.*}} "approx-func-fp-math"="true" {{.*}} + ---------------- bmahjour wrote: > can we avoid these attributes? 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 ``` ================ Comment at: clang/test/CodeGen/afn-flag-test.c:13 + // CHECK-NO-AFN: %{{.*}} = call double @{{.*}}exp{{.*}}(double %{{.*}}) + // CHECK-NO-AFN-NOT: attributes #0 ={{.*}} "approx-func-fp-math"="true" {{.*}} +} ---------------- bmahjour wrote: > avoid attributes. Same as above. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106191/new/ https://reviews.llvm.org/D106191 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits