awarzynski added inline comments.
================ Comment at: clang/include/clang/Driver/Options.td:1926 + " Default is 'fast' for CUDA, 'fast-honor-pragmas' for HIP, 'off' for flang, and 'on' otherwise.">, + HelpText<"Form fused FP ops (e.g. FMAs): fast | off (clang, flang), on | fast-honor-pragmas (clang only)">, Values<"fast,on,off,fast-honor-pragmas">; ---------------- As far as users are concerned, `flang-new` is just a Fortran compiler. Once we start adding references to Clang in `flang-new --help`, we might be confusing users (i.e. where's the boundary between Clang and Flang?) and exposing implementation details (i.e. that `flang-new` is implemented using `clangDriver`). Ideally we should avoid that. ================ Comment at: flang/include/flang/Frontend/LangOptions.h:29 + + // Enable the floating point pragma + FPM_On, ---------------- tblah wrote: > vzakhari wrote: > > tblah wrote: > > > vzakhari wrote: > > > > tblah wrote: > > > > > awarzynski wrote: > > > > > > What are these pragmas? Perhaps you can add a test that would > > > > > > include them? > > > > > I copied this comment from clang. I believe the pragma is > > > > > ``` > > > > > #pragma clang fp contract(fast) > > > > > ``` > > > > > > > > > > See > > > > > https://clang.llvm.org/docs/LanguageExtensions.html#extensions-to-specify-floating-point-flags > > > > > > > > > > This patch only adds support for argument processing, so I can't test > > > > > for the pragmas. > > > > I do not think we should blindly copy this from clang. I believe > > > > `-ffp-contract=on` is there to do the contraction complying to the > > > > language standard - but Fortran standard says nothing about > > > > contraction. I am also not aware about a Fortran compiler that > > > > supports directives related to contraction, so `fast-honor-pragmas` > > > > does not seem to be applicable as well. Basically, we end up with just > > > > `off` and `fast`. > > > > > > > > Now, it may be reasonable to support the same `-ffp-contract=` > > > > arguments so that users can apply the same options sets for C/C++ and > > > > Fortran compilations. If we want to do this, we need to map `on` and > > > > `fast-honor-pragmas` to something, e.g. `fast`. A driver warning (not > > > > an error) may be useful to make the option's behavior clear when `on` > > > > and `fast-honor-pragmas` are passed. > > > From the clang help text: > > > ``` > > > Form fused FP ops (e.g. FMAs): > > > fast (fuses across statements disregarding pragmas) > > > | on (only fuses in the same statement unless dictated by pragmas) > > > | off (never fuses) > > > Default is 'on' > > > ``` > > > > > > So if we removed `on` and set the default to `off` we would no longer > > > fuse within the same statement by default. > > > > > > Classic-flang seems to support `on`, `off` and `fast`: > > > https://github.com/flang-compiler/flang/blob/master/test/llvm_ir_correct/fma.f90 > > Not talking about defaults just yet, I think Flang cannot currently support > > the `on` mode as documented. > > > > I do not have the latest classic flang readily availalbe, but I am curious > > what it will generate for this example: > > ``` > > function fn(x,y,z) > > real :: x,y,z > > fn = x * y > > fn = fn + z > > end function > > ``` > > > > With a very old classic flang I get just `fast` math flags on the multiple > > and add instructions, which is obviously not what `on` is supposed to do: > > ``` > > $ flang -target aarch64-linux-gnu -O1 -c -S -emit-llvm -ffp-contract=on > > fma.f90 > > $ cat fma.ll > > %4 = fmul fast float %3, %1, !dbg !10 > > %5 = bitcast i64* %z to float*, !dbg !11 > > %6 = load float, float* %5, align 4, !dbg !11 > > %7 = fadd fast float %6, %4, !dbg !11 > > ``` > > > > Maybe the latest classic flang does support it properly, e.g. it only > > contracts operations from the same statement. But I do not see a way to > > support this in Flang right now, so documenting the `on` mode as it is in > > clang seems confusing. > > > > We can still support `on` in the Flang option, but I think we need to issue > > a warning saying that it defaults to something else, e.g. to `fast`. If > > mapping `on` to `fast` is not appropriate to some users, then they will > > have to explicitly specify `-ffp-contract=off` for Fortran compilations in > > their build system. > > > > I am also curious what `fuses in the same statement` means for Fortran. > > For example: > > ``` > > x1 = DOT_PRODUCT(x2, x3)+x4*x5+x6 > > ``` > > > > If Fortran processor decides to implement `DOT_PRODUCT` as inline > > multiply+add loop, does `-ffp-contract=on` apply to them or it only applies > > to `x4*x5+x6`? > Thanks for your feedback. > > I've updated my patch. Now flang only supports `off` and `fast`. The other > two map to `fast` and we default to `off`. > > gfortran seems to default to `fast`: > ``` > -ffp-contract=style > > -ffp-contract=off disables floating-point expression contraction. > -ffp-contract=fast enables floating-point expression contraction such as > forming of fused multiply-add operations if the target has native support for > them. -ffp-contract=on enables floating-point expression contraction if > allowed by the language standard. This is currently not implemented and > treated equal to -ffp-contract=off. > > The default is -ffp-contract=fast. > ``` > > https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html > gfortran seems to default to fast: Thanks for checking those docs - it's now becoming clear that `-ffp-contract=style` should only support: `on`, `off`, `fast`. https://reviews.llvm.org/D90174 introduced `fast-honor-pragmas` to solve a particular problem for a particular backend, i.e. [[ https://developer.amd.com/resources/rocm-learning-center/fundamentals-of-hip-programming/ | HIP ]]. From the commit message for D90174: > 'fast-honor-pragmas' is equivalent to 'fast' in frontend but let the backend > to use 'Standard' fp fuse option. To me this sounds like `--ffp-contract=fast-honor-pragrmas` should be replaced with: * `--ffp-contract=fast --ffp-fusion=standard` (i.e. add a new flag to provide extra control to the end users) * `--ffp-control=fast -x hip` --> `FPM_FastHonorPragmas` somewhere in Clang's [[ https://github.com/llvm/llvm-project/blob/74c2d4f6024c8f160871a2baa928d0b42415f183/clang/lib/Frontend/CompilerInvocation.cpp#L3965-L3966 | CompilerInvocartion.cpp ]] As in, I believe that the issue in HIP can be solved without `--ffp-contract=fast-honor-pragrmas`. And if we avoid that approach, we can have a flag that's consistent with GCC and also make the help text much clearer. @vzakhari > Now, it may be reasonable to support the same -ffp-contract= arguments so > that users can apply the same options sets for C/C++ and Fortran > compilations. Yes, but I am not aware of anyone experimenting with mixed-source projects yet. So currently `flang-new` should only be used for Fortran sources and `clang` for C family of languages. So I would leave that for later. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136080/new/ https://reviews.llvm.org/D136080 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
