This is an archive of the discontinued LLVM Phabricator instance.

[Legalizer] Fix fp-to-uint to fp-tosint promotion assertion.
ClosedPublic

Authored by timshen on Jan 4 2017, 3:38 AM.

Details

Summary

When promoting fp-to-uint16 to fp-to-sint32, the result is actually zero
extended. For example, given double 65534.0, without legalization:

fp-to-uint16: 65534.0 -> 0xfffe

With the legalization:

fp-to-sint32: 65534.0 -> 0x0000fffe

Without this patch, legalization wrongly emits a signed extend assertion,
which is consumed by later icmp instruction, and causes miscompile.

Note that the user needs to ensure that floating point value must be in [0, 65535),
otherwise we are allowed to have undefined behavior.

This patch reverts r279223 behavior and adds more tests and
documentations.

In PR29041's context, James Molloy mentioned that:

We don't need to mask because conversion from float->uint8_t is
undefined if the integer part of the float value is not representable in
uint8_t. Therefore we can assume this doesn't happen!

which is totally true and good, because fptoui is documented clearly to
have undefined behavior when overflow/underflow happens. We should take
the advantage of this behavior so that we can save unnecessary mask
instructions.

Event Timeline

timshen updated this revision to Diff 83026.Jan 4 2017, 3:38 AM
timshen retitled this revision from to [Legalizer] Fix fp-to-uint to fp-tosint promotion assertion..
timshen updated this object.
timshen added reviewers: jmolloy, nadav, echristo, kbarton.
timshen added a subscriber: llvm-commits.
timshen updated this object.Jan 4 2017, 3:39 AM
jmolloy accepted this revision.Jan 4 2017, 4:00 AM
jmolloy edited edge metadata.

Hi,

I agree with this. Thanks for fixing this!

James

This revision is now accepted and ready to land.Jan 4 2017, 4:00 AM
efriedma added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
436

Not a comment about your patch, exactly, but I don't think the assertion here is safe; AssertZext(UNDEF) can lead to undefined behavior. For example, LOAD(ADD(global_array, AND(SomeUndefValue, 255))) produces undef, assuming global_array has at least 256 elements. However, LOAD(ADD(global_array, AND(AssertZext(SomeUndefValue, i8), 255))) simplifies to LOAD(ADD(global_array, AssertZext(SomeUndefValue, i8))), which has undefined behavior.

timshen added inline comments.Jan 4 2017, 12:29 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
436

I agree, and I think being undefined in such a scenario matches part of the fptoui/fptosi contract:

If the value cannot fit in ty2, the results are undefined.

Since the value undef cannot fit in "ty2", the integer type, we allow undefined behavior here.

This revision was automatically updated to reflect the committed changes.