This is an archive of the discontinued LLVM Phabricator instance.

[LegalizeTypes] Fix saturation bug for smul.fix.sat
ClosedPublic

Authored by bjope on Jul 8 2019, 7:03 AM.

Details

Summary

Make sure we use SETGE instead of SETGT when checking
if the sign bit is zero at SMULFIXSAT expansion.

The faulty expansion occured when doing "expand" of
SMULFIXSAT and the scale was exactly matching the
size of the smaller type. For example doing

i64 Z = SMULFIXSAT X, Y, 32

and expanding X/Y/Z into using two i32 values.

The problem was that we sometimes did not saturate
to min when overflowing.

Here is an example using Q3.4 numbers:

Consider that we are multiplying X and Y.

X = 0x80 (-8.0 as Q3.4)
Y = 0x20 (2.0 as Q3.4)

To avoid loss of precision we do a widening
multiplication, getting a 16 bit result

Z = 0xF000 (-16.0 as Q7.8)

To detect negative overflow we should check if
the five most significant bits in Z are less than -1.
Assume that we name the 4 most significant bits
as HH and the next 4 bits as HL. Then we can do the
check by examining if
(HH < -1) or (HH == -1 && "sign bit in HL is zero").

The fault was that we have been doing the check as
(HH < -1) or (HH == -1 && HL > 0)
instead of
(HH < -1) or (HH == -1 && HL >= 0).

In our example HH is -1 and HL is 0, so the old
code did not trigger saturation and simply truncated
the result to 0x00 (0.0). With the bugfix we instead
detect that we should saturate to min, and the result
will be set to 0x80 (-8.0).

Diff Detail

Repository
rL LLVM

Event Timeline

bjope created this revision.Jul 8 2019, 7:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2019, 7:03 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
leonardchan accepted this revision.Jul 8 2019, 8:10 PM

LGTM. Good catch

This revision is now accepted and ready to land.Jul 8 2019, 8:10 PM
This revision was automatically updated to reflect the committed changes.