This is an archive of the discontinued LLVM Phabricator instance.

[Intrinsic] Expand SMULFIX to MUL, MULH[US], or [US]MUL_LOHI on vector arguments
ClosedPublic

Authored by leonardchan on Jan 20 2019, 3:31 PM.

Details

Summary

For zero scale SMULFIX, expand into MUL which produces better code for X86.

For vector arguments, expand into MUL if SMULFIX is provided with a zero scale. Otherwise, expand into MULH[US] or [US]MUL_LOHI,

Diff Detail

Repository
rL LLVM

Event Timeline

leonardchan created this revision.Jan 20 2019, 3:31 PM

How common would it be that the scale is zero? Is that really expected in reality or just in this kind of handwritten test cases?

Regardless, this patch does not introduce that much complexity so it does LGTM.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5373 ↗(On Diff #182719)

nit: This comment is a little bit confusing (does "here" refer to this method or the context for the legalization). Returning SDValue() will result in an unroll, so by doing that I think we will end up scalarizing the mulfix operation. I'd expect a comment explaining that we return SDValue() to get scalarization, but now the comment could be read as "avoid scalarization by returning SDValue()".

Maybe you can change the wording somehow, or simply remove the comment.

RKSimon added inline comments.Jan 21 2019, 2:52 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5397 ↗(On Diff #182719)

Why not let vectors try to use these cases and add a bail out?

} else if (VT.isVector()) {
  // Only expand vector types if we have the appropriate vector operations.
  return SDValue();
} else {
leonardchan marked 4 inline comments as done.

How common would it be that the scale is zero? Is that really expected in reality or just in this kind of handwritten test cases?

It's not as expected or important as with a non-zero scale, but it seems like a good case to cover. It's mostly just another test case to cover.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5373 ↗(On Diff #182719)

Removed the comment

5397 ↗(On Diff #182719)

Done. This also produces fewer instructions for the vector case with non-zero scale.

Please update the patch title/summary as its not just for zero scales any more.

RKSimon added inline comments.Jan 24 2019, 7:48 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1226 ↗(On Diff #182956)

Please can you rebase after rL352056 - I've renamed this getExpandedFixedPointMultiplication to expandFixedPointMul to match the other uses.

leonardchan retitled this revision from [Intrinsic] Expand vector SMULFIX to MUL on zero scale to [Intrinsic] Expand SMULFIX to MUL, MULH[US], or [US]MUL_LOHI on vector arguments.Jan 24 2019, 8:58 AM
leonardchan edited the summary of this revision. (Show Details)
leonardchan marked an inline comment as done.
RKSimon added inline comments.Jan 24 2019, 10:17 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
5367 ↗(On Diff #183327)

Doesn't this have to be:

if (!Scale) {
  if(VT.isVector() && !isOperationLegalOrCustom(ISD::MUL, VT)))
    return SDValue();
  return DAG.getNode(ISD::MUL, dl, VT, LHS, RHS);
}

otherwise scale == 0 cases can fall through and cause undef for out of range shifts.

leonardchan marked an inline comment as done.
This revision is now accepted and ready to land.Jan 29 2019, 12:53 AM
This revision was automatically updated to reflect the committed changes.