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

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:3159
+controlled with this pragma.
+``#pragma clang fp allow_reassociation`` allows control over the reassociation
+of floating point expressions. When enabled, this pragma allows the expression
----------------
I would say the previous name, `reassoc`, was more consistent. We do not use `allow_contraction`.


================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1336
+  "%select{invalid|missing}0 option%select{ %1|}0; expected contract or reassoc">;
+def err_pragma_fp_contract_invalid_argument : Error<
+  "unexpected argument '%0' to '#pragma clang fp contract'; "
----------------
erichkeane wrote:
> I think we'd want this combined with the one below.  You can use a %select to enable/disable 'fast'.  Something like:
> 
> "unexpected argument '%0' to '#pragma clang fp %select{contract|OTHERS}1; expected 'on'%select{, 'fast'|}1 or off"
There was discussion on the same topic in D65997. Probably code from there could be used here. The point is that this pragma is likely to get additional parameters in future and it would be nice if this implementation would be prepared to such extensions.


================
Comment at: clang/include/clang/Sema/Sema.h:9622
+
+  /// ActOnPragmaFPAllowReassociation - Called on well formed
+  /// \#pragma clang fp allow_reassociation
----------------
Duplication of function name in doc comments is outdated technique. Maybe it can be omitted? It would enhance readability a bit.


================
Comment at: clang/test/CodeGen/fp-reassoc-pragma-fails.cpp:1
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+float fp_reassoc_fail(float a, float b) {
----------------
This test has nothing with code generation. It should be moved to `Parser` directory. There is already a file  `pragma-fp.cpp` there, it could be amended with your tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78827