Page MenuHomePhabricator

Assume ieee behavior without denormal-fp-math attribute
Needs ReviewPublic

Authored by arsenm on Nov 7 2019, 11:09 PM.

Diff Detail

Event Timeline

arsenm created this revision.Nov 7 2019, 11:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2019, 11:09 PM
This revision is now accepted and ready to land.Nov 19 2019, 12:22 PM
cameron.mcinally requested changes to this revision.Nov 19 2019, 12:36 PM

Ah, I didn't realize there were test differences from this change. It should probably be combined with D69988...

This revision now requires changes to proceed.Nov 19 2019, 12:36 PM
arsenm updated this revision to Diff 230255.Nov 20 2019, 7:03 AM

Include x86 test updates

Attn: @craig.topper @eli.friedman. The FMF behavior looks like it will take a performance hit with the switch to default IEEE conformance. Is this something that needs to be publicized on llvm-dev before merging? Users who don't care will want to add an explicit denormal-fp-math attribute to their code.

Nit: the IR tests may be overly brittle -- checking for specific registers. It might be good to use update_test_checks.py so that we get the regex'd register expressions.

Attn: @craig.topper @eli.friedman. The FMF behavior looks like it will take a performance hit with the switch to default IEEE conformance. Is this something that needs to be publicized on llvm-dev before merging? Users who don't care will want to add an explicit denormal-fp-math attribute to their code.

I think this patch is dependent on D69979 (set the denorm attributes based on other clang behavior). We need a test-suite end-to-end test or at least have clang tests reference the expected codegen tests, so we know that handshake between front-end and codegen is working as expected.

Nit: the IR tests may be overly brittle -- checking for specific registers. It might be good to use update_test_checks.py so that we get the regex'd register expressions.

Agree - if possible, regenerate/add tests as NFC changes before this patch.

spatel added inline comments.Thu, Feb 13, 12:56 PM
llvm/test/CodeGen/X86/sqrt-fastmath-tune.ll
101

Does the "preseve" typo affect anything?

arsenm marked an inline comment as done.Thu, Feb 13, 2:25 PM
arsenm added inline comments.
llvm/test/CodeGen/X86/sqrt-fastmath-tune.ll
101

These aren't hitting the expansion, so I guess it doesn't. I assume this has something to do with not using reciprocal-estimates?

spatel added inline comments.Fri, Feb 14, 11:31 AM
llvm/test/CodeGen/X86/sqrt-fastmath-tune.ll
101

Yes, this file isn't well-specified about what it's trying to test for. I updated it here:
rG6071fc57a45f

That should remove it from this patch?

Independent of that - should we have an assert or warning that would catch typos in the attribute string?

arsenm marked an inline comment as done.Fri, Feb 14, 11:39 AM
arsenm added inline comments.
llvm/test/CodeGen/X86/sqrt-fastmath-tune.ll
101

I remember implementing a verifier check for this before ,but I can't seem to find the patch