This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv][X86][SystemZ] Use different algorithms for i64->double uint_to_fp under strictfp to avoid producing -0.0 when rounding toward negative infinity
ClosedPublic

Authored by craig.topper on Sep 3 2020, 3:39 PM.

Details

Summary

Some of our conversion algorithms produce -0.0 when converting unsigned i64 to double when the rounding mode is round toward negative. This switches them to other algorithms that don't have this problem. Since it is undefined behavior to change rounding mode with the non-strict nodes, this patch only changes the behavior for strict nodes.

There are still problems with unsigned i32 conversions too which I'll try to fix in another patch.

Fixes part of PR47393

Diff Detail

Event Timeline

craig.topper created this revision.Sep 3 2020, 3:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2020, 3:39 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
craig.topper requested review of this revision.Sep 3 2020, 3:39 PM
efriedma added inline comments.Sep 3 2020, 4:07 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2465

What types is this correct for? I guess the integer type has to be larger than the mantissa by some number of bits, so the divide by 2 doesn't affect the rounding?

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
6534

Should we drop the isStrictFPOpcode check later in the function?

uweigand added inline comments.Sep 4 2020, 3:32 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
6533

Why is this testing isStrictFPOpcode? The only difference between strict and non-strict operations is that the former respect exceptions (and current rounding mode, if applicable). The behavior of signed zeros is not affected by those; even a non-strict UINT_TO_FP should never produce a negative zero (unless the NoSignedZero flags is set, I guess).

craig.topper added inline comments.Sep 4 2020, 9:06 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
6533

The issue only occurs if the rounding mode has been changed to FE_DOWNWARD. Since it’s undefined behavior to change rounding mode and use non-strict unit_to_fp, I didn’t change those.

uweigand added inline comments.Sep 4 2020, 9:44 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
6533

Ah, I see. Thanks!

6556–6559

This comment is not accurate, then?

craig.topper retitled this revision from [FPEnv][X86][SystemZ] Use different algorithms for i64->double uint_to_fp under strictfp to avoid producing -0.0 when rounding to negative infinity to [FPEnv][X86][SystemZ] Use different algorithms for i64->double uint_to_fp under strictfp to avoid producing -0.0 when rounding toward negative infinity.
craig.topper edited the summary of this revision. (Show Details)

-Remove strict FP code that became dead with the new early outs
-Improve comments

efriedma accepted this revision.Oct 12 2020, 12:30 PM

LGTM with one minor comment.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2465

I'd like to see a comment briefly describing the widths where this works.

This revision is now accepted and ready to land.Oct 12 2020, 12:30 PM

Add the requested comment. @efriedma can you check my logic

efriedma added inline comments.Oct 21 2020, 2:45 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2465

Comment looks right.

This revision was landed with ongoing or failed builds.Oct 21 2020, 6:23 PM
This revision was automatically updated to reflect the committed changes.