[PATCH] D151834: Include math-errno with fast-math

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added inline comments.


================
Comment at: clang/include/clang/Basic/FPOptions.def:30
 OPTION(BFloat16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, FPEvalMethod)
+OPTION(MathErrno, bool, 1, BFloat16ExcessPrecision)
 #undef OPTION
----------------
Does this mean MathErrno is tracked in both LangOpts and FPOptions?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2248
       FD->hasAttr<AsmLabelAttr>() ? 0 : BuiltinID;
+  bool MathErrnoOverrride = false;
+  if (E->hasStoredFPFeatures()) {
----------------
You should add a comment here explaining what this is doing. I don't think it's obvious apart from the description of this patch.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2384
 
+  if (HasOptnone && !getLangOpts().MathErrno)
+    OptNone = false;
----------------
I don't understand what this is doing.


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:611
   void clear();
+  bool OptNone = true;
 
----------------
Why is this a module level flag, and why does it default to true?


================
Comment at: clang/test/CodeGen/math-errno.c:2
+// Tests that at -O2 math-errno is taken into account. Same than MSVC.
+// RUN: %clang_cc1 -Wno-implicit-function-declaration -fmath-errno \
+// RUN: -O2 -emit-llvm -o - %s \
----------------
Isn't math-errno true by default when fast-math isn't used?


================
Comment at: clang/test/CodeGen/math-errno.c:33
+
+__attribute__((optnone))
+float f4(float x) { 
----------------
Can you add a runline with -O0. That should prevent all instances of the intrinsics, right?


================
Comment at: clang/test/CodeGen/math-errno.c:49
+// FAST-LABEL: define dso_local nofpclass(nan inf) float @f4(float noundef nofpclass(nan inf) {{.*}})
+// FAST: call reassoc nnan ninf nsz arcp afn nofpclass(nan inf) float @sqrtf(float noundef nofpclass(nan inf) %0) #[[ATTR0:[0-9]+]]
+
----------------
I think the 'afn' flag here is a problem. The backend has no concept of errno, so 'afn' will be treated as allowing the function to be replaced.


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

https://reviews.llvm.org/D151834