[PATCH] D125625: Implementation of '#pragma STDC FENV_ROUND'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: zahiraam.
aaron.ballman added a comment.

Thank you for working on this (and thank you for your patience with how long it's taken me to get to this review)!

Adding another reviewer who has recently been poking at floating point environment pragmas in case Zahira has some thoughts here as well.



================
Comment at: clang/lib/CodeGen/CGStmt.cpp:488
+  }
+  if (NewRM != llvm::RoundingMode::Dynamic)
+    Builder.CreateIntrinsic(
----------------
I was imagining that if the new rounding mode is dynamic, we still need to reset the rounding mode back to what it was when the thread was created or to the previous rounding mode set by a library call, but now I'm not certain. I commented on one of the test cases about what I was thinking.


================
Comment at: clang/test/CodeGen/pragma-fenv_round.c:54-55
+// CHECK:  call float @llvm.experimental.constrained.fmul.f32({{.*}}, metadata !"round.towardzero", metadata !"fpexcept.ignore")
+// CHECK:  call void @llvm.set.rounding(i32 1)
+// CHECK:  call float @llvm.experimental.constrained.fadd.f32({{.*}}, metadata !"round.tonearest", metadata !"fpexcept.ignore")
+// CHECK:  call void @llvm.set.rounding(i32 0)
----------------
I was trying to figure out whether this was correct or not when I was looking at the codegen logic. C2x 7.6.2p3 says "... If no FENV_ROUND pragma is in effect, or the specified constant rounding mode is FE_DYNAMIC, rounding is according to the mode specified by the dynamic floating-point environment, which is the dynamic rounding mode that was established either at thread creation time or by a call to fesetround, fesetmode, fesetenv, or feupdateenv. ..." p2 says: "... When outside external declarations, the pragma takes effect from its occurrence until another FENV_ROUND pragma is encountered, or until the end of the translation unit. When inside a compound statement, the pragma takes effect from its first occurrence until another FENV_ROUND pragma is encountered (including within a nested compound statement), or until the end of the compound statement; ...". Finally, p4 says: "... Within the scope of an FENV_ROUND pragma establishing a mode other than FE_DYNAMIC, ... shall be evaluated according to the specified constant rounding mode (as though no constant mode was specified and the corresponding dynamic rounding mode had been established by a call to fesetround)."

I *think* this means that the rounding mode for `result += z;` should actually be `FE_TOWARDZERO`, but I'm not 100% sure. We enter the compound statement for the function body, then we enter a constant rounding mode of FE_TOWARDZERO. Then we get into a new nested compound statement, and we enter the rounding mode of FE_DYNAMIC, which says it should behave in the manner of the last call to fesetround, which was (as-if) called by the earlier pragma.

Thoughts?

I would be good to have a test for the behavior at file scope as well as at compound scope, e.g.,
```
float default_mode = 1.0f / 3.0f;

#pragma STDC FENV_ROUND FE_UPWARD  
float up = 1.0f / 3.0f;

#pragma STDC FENV_ROUND FE_DOWNWARD
float down = 1.0f / 3.0f;

#pragma STDC FENV_ROUND FE_DYNAMIC
float dynamic = 1.0f / 3.0f;
```
I'd expect the `up` and `down` cases to have different values, and I'd expect the `dynamic` case to either be the same as `down` or the same as `default_mode`.


================
Comment at: clang/test/CodeGen/pragma-fenv_round.c:71
+
+
+#pragma STDC FENV_ROUND FE_DOWNWARD
----------------
I think it'd be nice to add a test case here like:
```
float func_21(float x, float y) {
  return x + y;
}
```
to demonstrate that FE_DOWNWARD is still in effect within this function too.


================
Comment at: clang/test/CodeGen/pragma-fenv_round.c:92
+}
+
+// CHECK-LABEL: @func_22
----------------
Another fun test case to add would be a C++ case:
```
#pragma STDC FENV_ROUND FE_DOWNWARD
float func_cxx(float x, float y, float z = 1.0f/3.0f) {
  return x + y + z;
}

int main() {
#pragma STDC FENV_ROUND FE_UPWARD
  func_cxx(1.0f, 2.0f);
}
```
is `z` evaluated as upward (because default arguments are evaluated at the call site IIRC), or downward (because of the rounding mode when defining the function)?

I suppose another interesting case is:
```
inline float func(float x, float y) {
#pragma STDC FENV_ROUND FE_DOWNWARD
  return x + y;
}

int main(void) {
  func(1.0f, 2.0f);
  float f = 1.0f / 3.0f;
}
```
Even if the call to `func` is truly inlined, I still wouldn't expect `f` to be calculated in downward (unless that's the default rounding mode). Is that how you read the standard as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125625