Page MenuHomePhabricator

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

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

Details

Summary

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

Repository
rL LLVM

Event Timeline

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

Worth adding a vector test ?

bjope added a comment.Thu, Sep 5, 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.Fri, Sep 6, 3:51 AM

Makes sense - LGTM

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