[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/IR/FPState.h:1
+#ifndef LLVM_FPSTATE_H
+#define LLVM_FPSTATE_H
----------------
Missing license header and c++ mode comment


================
Comment at: llvm/lib/IR/FPState.cpp:1
+#include "llvm/IR/FPState.h"
+#include "llvm/IR/IRBuilder.h"
----------------
Missing license header


================
Comment at: llvm/lib/IR/FPState.cpp:73
+  if (IsConstrainedExcept && !IsConstrainedRounding) {
+    // If the rounding mode isn't set explicitly above, then use ebToNearest
+    // as the value when the constrained intrinsic is created
----------------
eb?


================
Comment at: llvm/unittests/IR/IRBuilderTest.cpp:205
+  V = Builder.CreateFAdd(V, V);
+  ASSERT_TRUE(!isa<ConstrainedFPIntrinsic>(V));
+
----------------
ASSERT_FALSE with no !


================
Comment at: llvm/unittests/IR/IRBuilderTest.cpp:217-218
+  ASSERT_TRUE(isa<ConstrainedFPIntrinsic>(V));
+  ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebStrict);
+  ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmDynamic);
+
----------------
Most of these should not be using ASSERT_, and instead EXPECT_EQ


================
Comment at: llvm/unittests/IR/IRBuilderTest.cpp:258
+  CII = cast<ConstrainedFPIntrinsic>(V);
+  ASSERT_TRUE(isa<ConstrainedFPIntrinsic>(V));
+  ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebMayTrap);
----------------
This already would have crashed from the cast<> before


================
Comment at: llvm/unittests/IR/IRBuilderTest.cpp:259-260
+  ASSERT_TRUE(isa<ConstrainedFPIntrinsic>(V));
+  ASSERT_TRUE(CII->getExceptionBehavior() == ConstrainedFPIntrinsic::ebMayTrap);
+  ASSERT_TRUE(CII->getRoundingMode() == ConstrainedFPIntrinsic::rmToNearest);
 
----------------
EXPECT_EQ


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62731