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

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

This patch contains many tests and they are valuable. However many of them are likely to be compatible with unpatched compiler, so are not related to casting nodes. I would propose to move such tests into a separate patch and make this one dependent on it. Leave here only tests that depend on the changes made by this patch.

Also tests should be as compact as possible. Test assertions should avoid checking unrelated nodes, as it increases the change of breaking the test and complicated its analysis.



================
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
----------------
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? 


================
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
+
----------------
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.


================
Comment at: clang/test/CodeGen/complex-strictfp.c:92-95
+  double _Complex a = 5;
+  double _Complex b = 42;
+
+  return a * b != b * a;
----------------
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.


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


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


================
Comment at: clang/test/CodeGen/complex-strictfp.c:360
+  cs += i;
+  D += cf;
+  cs /= ci1;
----------------
It seems only this statement involves floating point arithmetic. Others are integer only.


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


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


================
Comment at: clang/test/CodeGen/complex-strictfp.c:469-472
+  g1++;
+  g1--;
+  ++g1;
+  --g1;
----------------
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?


================
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
----------------
No FP casts.


================
Comment at: clang/test/CodeGen/incdec-strictfp.c:22
+
+  printf("%lf %lf\n", A, B);
+}
----------------
This line and corresponding function declaration seem unnecessary?


================
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
+
----------------
What is the effect of `-fsanitize=float-divide-by-zero`? If it absents does the test change behavior?


================
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;
----------------
These are integer values, they cannot produce constrained intrinsics. Why they are tested here?


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

https://reviews.llvm.org/D88913