mibintc marked an inline comment as done. mibintc added a subscriber: Anastasia. mibintc 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: > 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 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