[PATCH] D79903: FastMathFlags.allowContract should be init from FPFeatures.allowFPContractAcrossStatement

[John McCall via Phabricator via cfe-commits]

rjmccall 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:
> 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?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:460
   case PFC_Push:
-    Action = Sema::PSK_Push_Set;
-    FpPragmaStack.Act(Loc, Action, StringRef(), NewFPFeatures.getAsOpaqueInt());
+    if (FpPragmaStack.Stack.empty()) {
+      FpPragmaStack.Act(Loc, Sema::PSK_Set, StringRef(),
----------------
mibintc wrote:
> When i was adding a test, I realized that pragma float_control(push) then pop wasn't working as expected. If the stack is empty, which is most of the time, first need to push the current fp features onto the stack so they can be restored at the pop
It seems that the way `PragmaStack` is supposed to be used is that we're supposed to be using its `CurrentValue` instead of having a separate `CurFPFeatures`.  But mostly it looks like there's a lot of functionality associated with `PragmaStack` that we aren't using at all because this is a substantially simpler mode; we could really just be using a `SmallVector<FPOptions, 2>`.

Anyway, I guess this fix works, although it should be done in a separate patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79903/new/

https://reviews.llvm.org/D79903