[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

[Melanie Blower via Phabricator via cfe-commits]

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

I added inline comments describing what I did in this version of the patch to address the bug https://bugs.llvm.org/show_bug.cgi?id=44048



================
Comment at: clang/include/clang/AST/DeclBase.h:1539
+    /// Indicates if the function uses Floating Point Constrained Intrinsics
+    uint64_t UsesFPIntrin : 1;
   };
----------------
This corresponds to "strictfp" LLVM attribute. I add this here because I want to collect the information during Sema and set the attribute during CodeGen. The next thing I want to do is to add support for modifying float_control via pragma within function bodies (enable floating point control at the block level).  If I wasn't preparing to support floating_control via statement-level pragma then setting the bit could be accomplished entirely within CodeGen.


================
Comment at: clang/include/clang/AST/DeclBase.h:1560
     /// NumFunctionDeclBitfields or CXXConstructorDeclBitfields.
-    uint64_t NumCtorInitializers : 23;
+    uint64_t NumCtorInitializers : 22;
     uint64_t IsInheritingConstructor : 1;
----------------
Need to adjust the number of bits here, because it's at the threshold of overrunning 64 bits.


================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:444
 
+def warn_drv_experimental_fp_control_incomplete_opt : Warning<
+  "Support for floating point control option %0 is incomplete and experimental">,
----------------
@kpn thought it would be a good idea to add a Warning that the implementation of float control is experimental and partially implemented.  That's what this is for.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2345
+            << "-ffp-contract=fast";
+        optID = options::OPT_ffp_contract;
+        FPModel = Val;
----------------
michele.scandale wrote:
> Here the state of the floating point options seems unchanged except for `FPContract`. If I run `clang -ffp-model=fast -ffp-model=precise`, I would expect the state of the floating point options to match the one of `-fno-fast-math` except for `FPContract` which you want to be set to "fast".
> 
> I think you might need to replicate the reset for all the option here as well, so at this point I don't know how much worth is to use the optID reset trick for the "fast" case only.
@michele.scandale Thanks for your helpful review, I think I fixed the things that you remarked on. I also added a test case for the assertion fail that you saw.


================
Comment at: clang/test/CodeGen/fpconstrained.cpp:10
+
+  template <class>
+  class aaaa {
----------------
This is the test case from the bug report (null deref/segfault/in IRBuilder)


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:268
       I->addAttribute(AttributeList::FunctionIndex, Attribute::StrictFP);
-    setConstrainedFPFunctionAttr();
   }
----------------
@kpn I got rid of this line because the function attribute is being set in CodeGen


================
Comment at: llvm/unittests/IR/IRBuilderTest.cpp:186
   Builder.setIsFPConstrained(true);
+  auto Parent = BB->getParent();
+  Parent->addFnAttr(Attribute::StrictFP);
----------------
@kpn I changed the test to create the function attribute a priori since it will be set in CodeGen before passing to IRBuilder


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62731