[PATCH] D88913: [FPEnv] Use strictfp metadata in casting nodes

Wed Nov 4 08:49:21 PST 2020

kpn added a comment.

Update for review comments.

================
Comment at: clang/test/CodeGen/builtin_float_strictfp.c:1
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-windows-pc -ffp-exception-behavior=maytrap -o - %s | FileCheck %s --check-prefixes=CHECK,FP16
+// RUN: %clang_cc1 -emit-llvm -triple ppc64-be -ffp-exception-behavior=maytrap -o - %s | FileCheck %s --check-prefixes=CHECK,NOFP16
----------------
sepavloff wrote:
> Part of this test, namely functions `test_floats`, `test_doubles` and `tests_halves` must pass on unpatched compiler, because they don't contain conversion nodes. Maybe they can be extracted into separate patch, on which this one would depend? 
I deleted `test_floats` and `test_doubles` because of that lack of conversion nodes. But `test_half` does have implicit conversions and I added checks for more of them.

================
Comment at: clang/test/CodeGen/complex-math-strictfp.c:1
+// RUN: %clang_cc1 %s -ffp-exception-behavior=maytrap -O0 -emit-llvm -triple x86_64-unknown-unknown -o - | FileCheck %s --check-prefix=X86
+
----------------
sepavloff wrote:
> Throughout this test the only operation that involves complex is construction of complex value from real part. It does not use any conversion node, so this test must with unpatched compiler.
That's valid. The conversion from float to "_Complex float" does get an ImplicitCastExpr, but it shouldn't result in constrained intrinsics. So the test isn't needed. I'll nuke it.

================
Comment at: clang/test/CodeGen/complex-strictfp.c:92-95
+  double _Complex a = 5;
+  double _Complex b = 42;
+
+  return a * b != b * a;
----------------
sepavloff wrote:
> This function generated complicated code, which is hard to check and easy to break. Can this function be split into smaller functions which would test only one operation? It could allow to use more compact checks. I think these tests should pass on unpatched compiler.
I'll pull this out and put it in a subsequent patch. It shows we have work to do, but isn't needed for this patch.

================
Comment at: clang/test/CodeGen/complex-strictfp.c:131
+//
+void test2(int c) {
+  _Complex double X;
----------------
sepavloff wrote:
> This function does not produce constrained intrinsics at all.
Eliminated

================
Comment at: clang/test/CodeGen/complex-strictfp.c:239
+//
+void test3() {
+  g1 = g1 + g2;
----------------
sepavloff wrote:
> Can it be split into smaller functions?
Sure, can do.

================
Comment at: clang/test/CodeGen/complex-strictfp.c:397
+void t3() {
+  __complex__ long long v = 2;
+}
----------------
sepavloff wrote:
> Integer-only arithmetic.
Eliminated

================
Comment at: clang/test/CodeGen/complex-strictfp.c:421
+void t5() {
+  float _Complex x = t4();
+}
----------------
sepavloff wrote:
> No floating point operations, only moves.
Eliminated

================
Comment at: clang/test/CodeGen/complex-strictfp.c:469-472
+  g1++;
+  g1--;
+  ++g1;
+  --g1;
----------------
sepavloff wrote:
> Only these operations involve FP operations, but none produces constrained intrinsics. And they don't produce cast nodes. Besides, these inc/dec operations are represented in IR as add/sub, does it makes sense to test them separately?
Yes, a separate patch for those would be better.

================
Comment at: clang/test/CodeGen/exprs-strictfp.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -ffp-exception-behavior=maytrap -emit-llvm -o - | FileCheck %s
----------------
kpn wrote:
> sepavloff wrote:
> > No FP casts.
> There's an implied cast in the AST, and I'm pretty sure the bits from that are used when simplifying this into the fcmp. I'll doublecheck.
Confirmed. In `CodeGenFunction::EvaluateExprAsBool()` we set the constrained metadata and then call down eventually to `ScalarExprEmitter::EmitFloatToBoolConversion()`.

The node we get the metadata from is a "`ImplicitCastExpr 0x80fe2b338 'double' <LValueToRValue> FPExceptionMode=2`".

================
Comment at: clang/test/CodeGen/incdec-strictfp.c:22
+
+  printf("%lf %lf\n", A, B);
+}
----------------
sepavloff wrote:
> This line and corresponding function declaration seem unnecessary?
This whole test is better left to a subsequent patch. It's another holdover from my previous attempt at the code changes.

================
Comment at: clang/test/CodeGen/ubsan-conditional-strictfp.c:1
+// RUN: %clang_cc1 %s -triple x86_64-unknown-unknown -ffp-exception-behavior=maytrap -emit-llvm -fsanitize=float-divide-by-zero -o - | FileCheck %s
+
----------------
kpn wrote:
> sepavloff wrote:
> > What is the effect of `-fsanitize=float-divide-by-zero`? If it absents does the test change behavior?
> An earlier version of this patch did try passing bits all the way down to the calls to the IRBuilder. In this version of the patch the sanitizer was required to be involved to test one of the places that was changed. I had 100% test coverage for that version of the code.
> 
> I'll check and see if it is still needed.
Right, the test was meant to test a change to the ubsan block towards the top of `ScalarExprEmitter::EmitDiv()`. The change isn't present in this particular patch. We still have a problem here, but it doesn't involve casts, so this can wait for a future patch.

================
Comment at: clang/test/CodeGen/zvector-strictfp.c:9-23
+volatile vector signed char sc, sc2;
+volatile vector unsigned char uc, uc2;
+volatile vector bool char bc, bc2;
+
+volatile vector signed short ss, ss2;
+volatile vector unsigned short us, us2;
+volatile vector bool short bs, bs2;
----------------
kpn wrote:
> sepavloff wrote:
> > These are integer values, they cannot produce constrained intrinsics. Why they are tested here?
> This test is also a holdover from the first version of the patch. It was also needed to achieve 100% test coverage with that first version. I'll check and see if it is still needed in this v2 version of the patch.
It was needed to test one of the `CreateFAdd()` calls in `ScalarExprEmitter::EmitScalarPrePostIncDec()` but this is another case where there are no casts involved so this should go in a later patch.

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

https://reviews.llvm.org/D88913