Page MenuHomePhabricator

[FPEnv] Use strictfp metadata in casting nodes
ClosedPublic

Authored by kpn on Oct 6 2020, 12:05 PM.

Details

Summary

The strictfp metadata was added to the casting AST nodes in D85960, but we aren't using that metadata yet. This patch adds that support.

In order to avoid lots of ad-hoc passing around of the strictfp bits I updated the IRBuilder when moving from a function that has the Expr* to a function that lacks it. I believe we should switch to this pattern to keep the strictfp support from being overly invasive.

For the purpose of testing that we're picking up the right metadata, I also made my tests use a pragma to make the AST's strictfp metadata not match the global strictfp metadata. This exposes issues that we need to deal with in subsequent patches, and I believe this is the right method for most all of our clang strictfp tests.

Diff Detail

Event Timeline

kpn created this revision.Oct 6 2020, 12:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2020, 12:05 PM
kpn requested review of this revision.Oct 6 2020, 12:05 PM
kpn removed a project: Restricted Project.Oct 6 2020, 12:09 PM
kpn removed a subscriber: llvm-commits.
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2020, 12:09 PM
sepavloff added inline comments.Oct 7 2020, 4:12 AM
clang/lib/CodeGen/CodeGenFunction.cpp
145

Actually this is not correct, because subclass relations are not preserved in this case. For instance, CompounsAssignmentOperator is not handled, as it does not return BinaryOperatorClass.

I would recommend using Expr::getFPFeaturesInEffect or adding similar method.

clang/lib/Sema/SemaExpr.cpp
700

Can cast of kind CK_NullToPointer or CK_LValueToRValue depend on FP options?

rjmccall added inline comments.Oct 7 2020, 11:31 PM
clang/lib/Sema/SemaExpr.cpp
700

The first never involves an FP type.

The second can be a load of an FP type, but that should never be options-sensitive. I think it actually *can* do a FP operation because of half->float promotion, but I don't believe there are any options that ever affect that, as it's always perfectly value-preserving.

kpn added inline comments.Oct 8 2020, 10:48 AM
clang/lib/CodeGen/CodeGenFunction.cpp
145

That's much cleaner. Sorry I missed that earlier.

kpn updated this revision to Diff 297016.Oct 8 2020, 10:49 AM

Update/simplify for review comments.

sepavloff added inline comments.Oct 12 2020, 3:19 AM
clang/lib/CodeGen/CodeGenFunction.cpp
143–145

The local variable FPFeatures can be eliminated here.

153–154

This check can be lifted to just after the definition of OldFPFeatures.

clang/lib/CodeGen/CodeGenFunction.h
638–639

Is it possible to merge this variables into OldFPFeatures? In what cases state of Builder may differ from the state of CGF.CurFPFeatures?

clang/test/CodeGen/aarch64-v8.2a-neon-intrinsics-constrained.c
6

Why did you change exception behavior to maytrap?

26

Why they are wrong?

kpn added inline comments.Oct 13 2020, 9:56 AM
clang/lib/CodeGen/CodeGenFunction.h
638–639

Without this patch they will differ if the AST changes CGF.CurFPFeatures and the Builder hasn't been updated yet. Say, because we're going back up the tree.

The concern is that we'll miss places that need to be updated to respect the AST. But if we don't make a point of resetting the Builder then we'll end up with stale data in the Builder that "just happens" to work. We need tests to actually test what they are supposed to test without relying on stale data.

The comments in the tests about "maytrap" being wrong show us that we absolutely have exactly this bug in multiple places. The Builder isn't getting properly updated and this change makes that visible and clear.

clang/test/CodeGen/aarch64-v8.2a-neon-intrinsics-constrained.c
6

Because "maytrap" is not the IRBuilder's default of "strict" and it isn't clang's default of "ignore".

The #pragma sets the exception handling to "strict". Having the command line arguments set a global "maytrap" with a #pragma that sets it to "strict" at the top of the file means that places in clang that need to be updated will become visible.

26

Because the #pragma covers the entire file and sets exception handling to "strict". Thus all constrained intrinsic calls should be "strict", and if they are "maytrap" or "ignore" then we have a bug.

kpn updated this revision to Diff 297927.Oct 13 2020, 11:41 AM

Updates from review.

sepavloff added inline comments.Oct 19 2020, 5:15 AM
clang/test/CodeGen/aarch64-v8.2a-neon-intrinsics-constrained.c
26

What is the reason for that? Does #pragma float_control work incorrectly? Why in clang/test/CodeGen/complex-math-strictfp.c exception handling is correct?

kpn added inline comments.Oct 23 2020, 10:54 AM
clang/lib/CodeGen/CodeGenFunction.cpp
153–154

Actually, no, because this leaves OldExcept and OldRounding uninitialized when they get used by the destructor.

clang/test/CodeGen/aarch64-v8.2a-neon-intrinsics-constrained.c
26

The #pragma works just fine. The problem is that we need to get the strictfp bits from the AST to the IRBuilder, and we haven't finished implementing that. So sometimes it works, like in complex-math-strictfp.c, and sometimes it doesn't. As you can see in the tests in this patch.

kpn updated this revision to Diff 300346.Oct 23 2020, 10:57 AM

Fix a bug introduced in the last round, and eliminate the pass manager argument from one of the tests.

kpn added inline comments.Oct 23 2020, 11:01 AM
clang/test/CodeGen/aarch64-v8.2a-neon-intrinsics-constrained.c
26

I forgot to mention that complex-math-strictfp.c gets a correct BinOpInfo passed down to get the IRBuilder set correctly. But there are other places that don't correctly set BinOpInfo and so we get inconsistent behavior despite the call to the IRBuilder being wrapped in FPOptsRAII. And _that's_ why I structured this patch the way I did.

Generally the patch looks good. But the need to expect incorrect values in tests is a concern. Maybe this is a consequence of storing exception behavior in a separate field of CGFPOptionsRAII. This misbehavior should be fixed.

clang/test/CodeGen/aarch64-v8.2a-neon-intrinsics-constrained.c
26

The problem is that we need to get the strictfp bits from the AST to the IRBuilder

What is the status of this problem? Is it on track?

kpn added a comment.Oct 27 2020, 5:25 AM

Generally the patch looks good. But the need to expect incorrect values in tests is a concern. Maybe this is a consequence of storing exception behavior in a separate field of CGFPOptionsRAII. This misbehavior should be fixed.

In this patch? Because that's going to be a huge patch. Fixing all the issues with strictfp AST->IRBuilder is going to be large. Fixing them incrementally seems better to me. Eliminating the incorrect values in the tests marked FIXME would sweep the problem under the rug.

It's true that incorrect values in tests is _not_ the desired end state. But it seems to me that as a _temporary_ incremental step it beats the alternatives.

clang/test/CodeGen/aarch64-v8.2a-neon-intrinsics-constrained.c
26

This is a larger problem that we need to tackle incrementally. I'd like to get a signoff on the approach taken in this patch before going too far down the road of writing fixes for all the issues we have here. But it is at the top of my list.

In D88913#2356152, @kpn wrote:

Generally the patch looks good. But the need to expect incorrect values in tests is a concern. Maybe this is a consequence of storing exception behavior in a separate field of CGFPOptionsRAII. This misbehavior should be fixed.

In this patch? Because that's going to be a huge patch. Fixing all the issues with strictfp AST->IRBuilder is going to be large. Fixing them incrementally seems better to me. Eliminating the incorrect values in the tests marked FIXME would sweep the problem under the rug.

It's true that incorrect values in tests is _not_ the desired end state. But it seems to me that as a _temporary_ incremental step it beats the alternatives.

OK. Users that called compiler with -ffp-exception-behavior=strict shouldn't notice any changes, right? Other non-default FP mode combinations anyway doesn't work now. So this patch shouldn't break existing code.

The only missing thing is tests that check non-default rounding mode. #pragma STDC FENV_ROUND can be used to prepare them, as it is done in D90026. Could you please add such test, to check if rounding mode is correctly processed in cast nodes?

kpn updated this revision to Diff 301395.Oct 28 2020, 12:54 PM

Add some tests for changing the rounding metadata.

These tests brought out some places that needed to set the strictfp metadata, so I've added those as well.

kpn updated this revision to Diff 301695.Oct 29 2020, 11:19 AM

Fix test, and eliminate a debugging "tee" command accidentally left in.

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.

clang/test/CodeGen/builtin_float_strictfp.c
2

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?

clang/test/CodeGen/complex-math-strictfp.c
1 ↗(On Diff #301695)

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.

clang/test/CodeGen/complex-strictfp.c
93–96

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.

132

This function does not produce constrained intrinsics at all.

240

Can it be split into smaller functions?

361

It seems only this statement involves floating point arithmetic. Others are integer only.

398

Integer-only arithmetic.

422

No floating point operations, only moves.

470–473

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?

clang/test/CodeGen/exprs-strictfp.c
2

No FP casts.

clang/test/CodeGen/incdec-strictfp.c
22 ↗(On Diff #301695)

This line and corresponding function declaration seem unnecessary?

clang/test/CodeGen/ubsan-conditional-strictfp.c
1 ↗(On Diff #301695)

What is the effect of -fsanitize=float-divide-by-zero? If it absents does the test change behavior?

clang/test/CodeGen/zvector-strictfp.c
9–23 ↗(On Diff #301695)

These are integer values, they cannot produce constrained intrinsics. Why they are tested here?

kpn added a comment.Oct 30 2020, 7:54 AM

I'll see how much I can simplify of the tests.

clang/test/CodeGen/exprs-strictfp.c
2

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.

clang/test/CodeGen/ubsan-conditional-strictfp.c
1 ↗(On Diff #301695)

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.

clang/test/CodeGen/zvector-strictfp.c
9–23 ↗(On Diff #301695)

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.

kpn added a comment.Wed, Nov 4, 8:49 AM

Update for review comments.

clang/test/CodeGen/builtin_float_strictfp.c
2

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.

clang/test/CodeGen/complex-math-strictfp.c
1 ↗(On Diff #301695)

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.

clang/test/CodeGen/complex-strictfp.c
93–96

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.

132

Eliminated

240

Sure, can do.

398

Eliminated

422

Eliminated

470–473

Yes, a separate patch for those would be better.

clang/test/CodeGen/exprs-strictfp.c
2

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".

clang/test/CodeGen/incdec-strictfp.c
22 ↗(On Diff #301695)

This whole test is better left to a subsequent patch. It's another holdover from my previous attempt at the code changes.

clang/test/CodeGen/ubsan-conditional-strictfp.c
1 ↗(On Diff #301695)

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.

clang/test/CodeGen/zvector-strictfp.c
9–23 ↗(On Diff #301695)

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.

kpn updated this revision to Diff 302860.Wed, Nov 4, 8:51 AM

Update for review comments.

sepavloff accepted this revision.Fri, Nov 6, 6:13 AM

LGTM.

This revision is now accepted and ready to land.Fri, Nov 6, 6:13 AM
This revision was automatically updated to reflect the committed changes.