Page MenuHomePhabricator

[FPEnv] Correct constrained metadata in fp16-ops-strict.c
ClosedPublic

Authored by kpn on Dec 3 2020, 12:08 PM.

Details

Summary

This test shows we're in some cases not getting strictfp information from the AST. Correct that.

Diff Detail

Event Timeline

kpn requested review of this revision.Dec 3 2020, 12:08 PM
kpn created this revision.
mibintc added inline comments.Dec 6 2020, 5:09 AM
clang/lib/CodeGen/CGExprScalar.cpp
2992

did you mean to leave this here? (blame shows the fixme comment dates from 2012)

3005

What's the rule to follow about when we need to FPOptsRAII?

kpn added inline comments.Dec 7 2020, 7:31 AM
clang/lib/CodeGen/CGExprScalar.cpp
2992

I was hoping someone knew what "floating point environment" is relevant here. From reading the commit message it doesn't sound like it matters to us, but I thought I'd flag it anyway.

No, I wasn't planning on committing with this comment.

3005

It is used on the border between code that has the AST node and code that doesn't. If any code below this point might use the constrained floating point intrinsics then the FPOptsRAII is needed.

Sometimes this border is at a call to the IRBuilder. Sometimes it's buried elsewhere. The hope is that by having the location be defined there we can at some point audit to verify we have it everywhere we should.

mibintc added inline comments.Dec 7 2020, 7:40 AM
clang/lib/CodeGen/CGExprScalar.cpp
3005

Oh, that doesn't sound very bug-proof. Do you mind pointing out 2 different instances, one "AST node" and one "buried elsewhere"? Or there's probably a code review I should read to find this? Thank you

kpn added inline comments.Dec 7 2020, 8:12 AM
clang/lib/CodeGen/CGExprScalar.cpp
3005

Agreed, but the approach of wrapping calls to the IRBuilder isn't very bug-proof either. And it requires threading strictfp info throughout code that otherwise doesn't support carrying around info currently. So it would be invasive and hard to audit.

This patch is an example of code that has the AST node (the Expr*) and is calling the IRBuilder. For "buried" examples see D88913.

mibintc accepted this revision.Dec 7 2020, 8:19 AM

Thanks for the additional info @kpn

This revision is now accepted and ready to land.Dec 7 2020, 8:19 AM
This revision was landed with ongoing or failed builds.Dec 8 2020, 7:18 AM
This revision was automatically updated to reflect the committed changes.