[PATCH] D78827: Add support for #pragma clang fp reassociate(on|off) -- floating point control of associative math transformations

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Just minor requests now.



================
Comment at: clang/docs/LanguageExtensions.rst:3177
+Both floating point reassociation and floating point contraction can be
+controlled with this pragma.
+``#pragma clang fp reassoc`` allows control over the reassociation
----------------
rjmccall wrote:
> Let's go ahead and word this as if arbitrary things will be controllable in the future.  So:
> 
> > Currently, the following things can be controlled by this pragma:
Thanks.  Please put the first sentence in its own paragraph and end it with a colon so that it logically leads into both of the following blocks.


================
Comment at: clang/docs/LanguageExtensions.rst:3182
+enabled for the translation unit with the ``-fassociative-math`` flag.
+The pragma can take two values: ``on`` and ``off``.
+
----------------
Do you want to add an example here?


================
Comment at: clang/docs/LanguageExtensions.rst:3192
 in cases when the language standard does not make this possible (e.g. across
 statements in C)
 
----------------
Oh, and there's a missing period here.


================
Comment at: clang/include/clang/Basic/LangOptions.h:186
+    FPM_Fast
   };
 
----------------
mibintc wrote:
> rjmccall wrote:
> > I'm not sure I think this fusion was an improvement; the net effect was to remove a few lines from this header and make a bunch of switches unnecessarily non-exhaustive.
> I dropped the on/off enumeration and just using boolean, where needed, to show the setting, do you like this better? 
Yeah, thanks.


================
Comment at: clang/include/clang/Sema/Sema.h:9624
+  /// \#pragma clang fp reassociate
+  void ActOnPragmaFPAllowReassociation(bool IsEnabled);
 
----------------
Please name this the same as the language feature.


================
Comment at: clang/lib/Parse/ParsePragma.cpp:2900
       return;
     }
     PP.Lex(Tok);
----------------
Please make this a single block by making the condition something like `if (!FlagValue || (FlagKind == TokFPAnnotValue::Reassociate && FlagValue == TokFPAnnotValue::Fast))`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78827