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