mibintc added a subscriber: arsenm. 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); ---------------- 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. 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
