[PATCH] D93769: [clang] Add support for option -ffp-eval-method and extend #pragma float_control similarly

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 9 inline comments as done.
mibintc added inline comments.


================
Comment at: clang/include/clang/Lex/Preprocessor.h:181
   IdentifierInfo *Ident__is_target_environment;    // __is_target_environment
+  IdentifierInfo *Ident__FLT_EVAL_METHOD__;        // __FLT_EVAL_METHOD__
 
----------------
aaron.ballman wrote:
> Otherwise, when `LexExpandSpecialBuiltins` is `false`, I think this winds up being an uninitialized pointer value.
thanks for catching this!


================
Comment at: clang/lib/Lex/PPMacroExpansion.cpp:1714
       });
-  } else if (II == Ident__has_cpp_attribute ||
-             II == Ident__has_c_attribute) {
+  } else if (II == Ident__has_cpp_attribute || II == Ident__has_c_attribute) {
     bool IsCXX = II == Ident__has_cpp_attribute;
----------------
aaron.ballman wrote:
> It looks like some unrelated formatting changes snuck in.
clang-format really wants to fix this file


================
Comment at: clang/lib/Sema/Sema.cpp:216
+    // Use setting from TargetInfo.
+    PP.setCurrentFltEvalMethod(static_cast<LangOptions::FPEvalMethodKind>(
+        ctxt.getTargetInfo().getFloatEvalMethod()));
----------------
aaron.ballman wrote:
> I don't think this cast is needed, is it? (It might make sense for the `getFloatEvalMethod()` return type to be `int` rather than `unisnged` though, given that we support negative values for implementation-defined semantics?)
You are so right. i changed the type and modified the identifier like you suggested to make things more easily understandable


================
Comment at: clang/test/CodeGen/fp-floatcontrol-pragma.cpp:240
+
+float mySub(float x, float y) {
+  // CHECK: define {{.*}}float {{.*}}mySub{{.*}}
----------------
aaron.ballman wrote:
> Can we also get some tests that show how this behaves with the less common floating-point types like float16, bfloat16, float128, half?
I added tests __fp16 and __float128. I don't know if the other type names are supported in Intel, _Float16 isn't supported. I didn't see a typename half nor bfloat16 being used in any of the CodeGen tests, is this sufficient? if the predicate IsFloatingType is true and if the expression comes into UsualUnaryConversions then this new code should take effect 


================
Comment at: clang/test/Preprocessor/predefined-flteval-macro.c:1
+// RUN: %clang_cc1 -std=c11  -E -triple=aarch64 -xc  %s | FileCheck %s
+// RUN: %clang_cc1 -std=c11  -triple=aarch64 -xc  -fsyntax-only %s
----------------
Since __FLT_EVAL_METHOD__ is no longer a typical predefined macro, I removed all the tests from clang/test/Preprocessor/init.c (see above) and created this to test all the combinations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93769