[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

[Melanie Blower via Phabricator via cfe-commits]

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

In D87528#2270541 <https://reviews.llvm.org/D87528#2270541>, @rsmith wrote:

>> @rsmith asked "I don't see changes to the constant evaluator". The semantic rules for enabling this pragma require that strict or precise semantics be in effect., see SemaAttr.cpp And the floating point constant evaluation doesn't occur when strict or precise
>
> What do you mean "the floating point constant evaluation doesn't occur when strict or precise"? I don't see any code to do that in `ExprConstant.cpp`, neither in trunk nor in this patch. The constant evaluator certainly is still invoked when we're in strict or precise mode. I would expect the evaluator would need to check for FP strictness whenever it encounters a floating-point operator (eg, here: https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ExprConstant.cpp#L13340), and treat such cases as non-constant.

1.0/3.0 is now static folded.  @rsmith Did you change your mind about this?  Is static folding what you want here?

> I'd like to see a C++ test for something like this:
>
>   float myAdd() {
>   #pragma STDC FENV_ACCESS ON
>       static double v = 1.0 / 3.0;
>       return v;
>   }
>
> If that generates a runtime guarded initialization for `v`, then I'd be a bit more convinced we're getting this right.

Some inline comments, still owe you a list of test that fails.  check-clang runs clean with this patch



================
Comment at: clang/lib/AST/ExprConstant.cpp:2496-2498
+  if (llvm::APFloatBase::opOK != Result.convertFromAPInt(Value,
+                                   Value.isSigned(),
+                                   APFloat::rmNearestTiesToEven) &&
----------------
rsmith wrote:
> Following D89360, we should skip this check if `Info.InConstantContext`.
I tried making this change but I got about 30 lit fails, maybe my patch was incorrect. I'll rerun with the patch and post the list of fails. 


================
Comment at: clang/test/CodeGen/fp-floatcontrol-pragma.cpp:159
+  //CHECK: store float 3.0{{.*}}retval{{.*}}
+  static double v = 1.0 / 3.0;
+  //CHECK-FENV: llvm.experimental.constrained.fptrunc.f32.f64{{.*}}
----------------
@rsmith - the division is constant folded.  is that right? 


================
Comment at: clang/test/Parser/pragma-fenv_access.c:28
+#if defined(CPP) & defined(STRICT)
+//not-expected-error at +3 {{constexpr variable 'frac' must be initialized by a constant expression}}
+//not-expected-note at +2 {{compile time floating point arithmetic suppressed in strict evaluation modes}}
----------------
@rsmith no diagnostic, is this OK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87528