Page MenuHomePhabricator

[CodeGen] Handle SMULFIXSAT with scale zero in TargetLowering::expandFixedPointMul

Authored by bjope on Sep 2 2019, 4:44 AM.



Normally TargetLowering::expandFixedPointMul would handle
SMULFIXSAT with scale zero by using an SMULO to compute the
product and determine if saturation is needed (if overflow
happened). But if SMULO isn't custom/legal it falls through
and uses the same technique, using MULHS/SMUL_LOHI, as used
for non-zero scales.

Problem was that when checking for overflow (handling saturation)
when not using MULO we did not expect to find a zero scale. So
we ended up in an assertion when doing

APInt::getLowBitsSet(VTSize, Scale - 1)

This patch fixes the problem by adding a new special case for
how saturation is computed when scale is zero.

Diff Detail


Event Timeline

bjope created this revision.Sep 2 2019, 4:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2019, 4:44 AM

Worth adding a vector test ?

bjope added a comment.Sep 5 2019, 3:20 PM

Worth adding a vector test ?

VectorLegalizer::Expand is still doing unroll instead of calling ExpandFixedPointMul for SMULFIXSAT. I do not think that we currently ever get here with a vector type (with saturation). The only point with adding a vector test would be to have one for completion, in case we want to change that behavior in the future. So I don't think that it is relevant for the bugfix.

RKSimon accepted this revision.Sep 6 2019, 3:51 AM

Makes sense - LGTM

This revision is now accepted and ready to land.Sep 6 2019, 3:51 AM
This revision was automatically updated to reflect the committed changes.