This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Update `isKnownNeverNaN` for `FMA/FMAD`
ClosedPublic

Authored by Pierre-vh on Sep 29 2022, 12:46 AM.

Details

Summary

We can still get a NaN even if none of the operands are NaN,
e.g. from +inf/-inf. D50804 didn't catch that.

Diff Detail

Event Timeline

Pierre-vh created this revision.Sep 29 2022, 12:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 12:46 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Pierre-vh requested review of this revision.Sep 29 2022, 12:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 12:46 AM
foad added a comment.Sep 29 2022, 1:27 AM

Code change looks good. Is it possible to add a test as described in https://reviews.llvm.org/D50804#1202429?

Code change looks good. Is it possible to add a test as described in https://reviews.llvm.org/D50804#1202429?

Do you mean we need the same test but in AMDGPU, or does the test need to be changed as well?

foad added a comment.Sep 29 2022, 2:01 AM

Code change looks good. Is it possible to add a test as described in https://reviews.llvm.org/D50804#1202429?

Do you mean we need the same test but in AMDGPU, or does the test need to be changed as well?

The test added in D50804 was for fadd and fmul. I'm suggesting you could extend it now to cover fma and fmad as well.

I did some tries but I can't get a DAG combine to involve FMA/FMAD in DAGISel that's affected by this change (I tried both on AArch64 and AMDGPU). Any idea of how I can write such a test and what the expected output should be?
The only combine I found involving this function only works if -enable-no-signed-zeros-fp-math is used

arsenm added a comment.Oct 5 2022, 7:36 AM

I did some tries but I can't get a DAG combine to involve FMA/FMAD in DAGISel that's affected by this change (I tried both on AArch64 and AMDGPU). Any idea of how I can write such a test and what the expected output should be?
The only combine I found involving this function only works if -enable-no-signed-zeros-fp-math is used

Then use "no-signed-zeros-fp-math"="true" in the test. That combine should also be updated to use nsz flags

Pierre-vh updated this revision to Diff 465405.Oct 5 2022, 8:07 AM

Add fma test

arsenm accepted this revision.Oct 5 2022, 8:14 AM
This revision is now accepted and ready to land.Oct 5 2022, 8:14 AM
This revision was automatically updated to reflect the committed changes.