Anastasia added inline comments.
================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2943 + if (Opts.FastRelaxedMath) + Opts.setDefaultFPContractMode(LangOptions::FPM_Fast); Opts.HexagonQdsp6Compat = Args.hasArg(OPT_mqdsp6_compat); ---------------- mibintc wrote: > mibintc wrote: > > rjmccall wrote: > > > michele.scandale wrote: > > > > mibintc wrote: > > > > > mibintc wrote: > > > > > > rjmccall wrote: > > > > > > > mibintc wrote: > > > > > > > > rjmccall wrote: > > > > > > > > > mibintc wrote: > > > > > > > > > > I changed this because the FAST version of this test > > > > > > > > > > clang/test/CodeGenOpenCL/relaxed-fpmath.cl wants the 'fast' > > > > > > > > > > attribute on the instruction dump. All the LLVM FMF bits > > > > > > > > > > must be set for the fast attribute print. By default, the > > > > > > > > > > value for OpenCL is ffp-contract=on > > > > > > > > > Is there an overall behavior change for OpenCL across these > > > > > > > > > patches? > > > > > > > > I think the answer to your question is no. Here is more > > > > > > > > details: OpenCL sets the default behavior to ffp-contract=on. > > > > > > > > In https://reviews.llvm.org/D72841 I added the function > > > > > > > > UpdateFastMathFlags, that function set the llvm > > > > > > > > FMF.allowContract bit to be ( ffp-contract=on or > > > > > > > > ffp-contract=fast). This patch just drops the check on > > > > > > > > ffp-contract=on. I didn't wind back to see how the llvm fast > > > > > > > > attribute was being set for this [opencl] test case originally. > > > > > > > Well, to what extent are there (including this patch) overall > > > > > > > test changes for the OpenCL tests, and what are tthey? > > > > > > In the #pragma float_control patch https://reviews.llvm.org/D72841, > > > > > > I changed 2 CodeGen/OpenCL tests: relaxed-fp-math.cl in the > > > > > > non-FAST cases to show the contract bit. Also 1 line in > > > > > > single-precision-constant.cl for the same reason, to show the > > > > > > contract bit. This patch undoes those test changes. I'll do more > > > > > > investigation to understand why the fast bit isn't being set in the > > > > > > FAST case in relaxed-fpmath.cl without the change to > > > > > > CompilerInovcaton > > > > > Prior to the patch for #pragma float_control, the llvm.FastMathFlags > > > > > was initialized from LangArgs.FastMath in the CodeGenFunction > > > > > constructor approximately line 74, and the FMF values in IRBuilder > > > > > were never changed--For the test > > > > > clang/test/CodeGenOpenCL/relaxed-fpmath.cl with option > > > > > -cl-fast-relaxed-math. (In ParseLangArgs, Opts.FastMath = > > > > > Args.hasArg(OPT_ffast_math) || > > > > > Args.hasArg(OPT_cl_fast_relaxed_math)) If FastMath is on, then all > > > > > the llvm.FMF flags are set. > > > > > > > > > > The #pragma float_control patch does change the IRBuilder settings in > > > > > the course of visiting the Expr nodes, using the information in the > > > > > Expr nodes, but the initial setting of FPFeatures from the command > > > > > line overlooked the fact that FastMath, and therefore > > > > > ffp-contract=fast, is enabled. > > > > Prior to D72841 compiling something with `-ffast-math > > > > -ffp-contract={on,off}` was still producing `fast` as fast-math flags > > > > on the instructions for the same reason. > > > > The clang driver does not consider the contraction mode for passing > > > > `-ffast-math` to CC1, which is consistent with the GCC behavior (I > > > > checked if the `__FAST_MATH__` is defined if I compile with > > > > `-ffast-math -ffp-contract=off`). > > > > According to this, the code in `CodeGenFunction`: > > > > ``` > > > > if (LangOpts.FastMath) > > > > FMF.setFast(); > > > > ``` > > > > is not correct. > > > > > > > > The OpenCL option `-cl-fast-relaxed-math` should be equivalent to > > > > `-ffast-math` for the user. I don't know what the interaction between > > > > `-cl-fast-relaxed-math` and `-ffp-contract={on, off,fast}`. > > > > > > > > If we need to keep `-cl-fast-relaxed-math` a CC1 option, I guess the > > > > following might work: > > > > ``` > > > > if (Args.hasArg(OPTS_cl_fast_relaxed_math) && > > > > !Arg.hasArg(OPT_ffp_contractEQ)) > > > > Opts.setDefaultFPContractMode)LangOptions::FPM_Fast); > > > > ``` > > > Okay, thanks for the investigation, I agree that that all makes sense. > > > I'm not sure what it would mean to enable fast math but disable > > > contraction — that combination doesn't seem useful. > > > > > > I'm fine with going forward with the OpenCL changes as they are, but you > > > might want to specifically reach out to the OpenCL people and let them > > > know what's going on. > > @arsenm Matt, I'm making this change to CompilerInvocation to fix a bug > > that was introduced by https://reviews.llvm.org/D72841 I see that you > > contribute to OpenCL will you please let interested folks know about this > > change? > > D72841 erroneously set the llvm.FMF.contract bit to true whether the > > contraction status was "allow contraction across statements" or "allow > > contraction within statements". This patch corrects that error to set > > llvm.FMF.contract bit only when contraction status is "allow contraction > > across statements". D72841 mis-captured the initial contraction status, > > this patch corrects that by checking the specific OpenCL option > > OPTS_cl_fast_relaxed_math. > > D72841 made a couple changes to OpenCL tests about the 'contract' flag in > > the IR, this patch undoes those test changes. > @Anastasia Adding Anastasia about this issue This change seems reasonable as it indeed restores the correct OpenCL behavior. Thanks for looking into this! I think we might also be able to set `contract` on `-cl-mad-enable`, but this can be done as a separate follow up improvement patch if we decide this is the right route forward. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79903/new/ https://reviews.llvm.org/D79903 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
