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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
