This is an archive of the discontinued LLVM Phabricator instance.

[SDAG] Support vector UMULO/SMULO
ClosedPublic

Authored by nikic on Feb 9 2019, 5:43 AM.

Details

Summary

Second part of https://bugs.llvm.org/show_bug.cgi?id=40442.

I've added an UnrollVectorOverflowOp method on SelectionDAG that follows the existing UnrollVectorOp method, but maybe that's too specific and should just be a private helper for the TargetLowering expansion code?

Diff Detail

Repository
rL LLVM

Event Timeline

nikic created this revision.Feb 9 2019, 5:43 AM
nikic updated this revision to Diff 187172.Feb 17 2019, 12:55 PM

Rebase over D58006.

RKSimon added inline comments.Feb 18 2019, 3:44 AM
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
981 ↗(On Diff #187172)

This needs fixing right away as a separate commit - we should be using getShiftAmountTy. I'm happy for you to use getScalarSizeInBits straight away as well.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
8953 ↗(On Diff #187172)

Use ExtractVectorElements ?

8963 ↗(On Diff #187172)

Use append()?

lib/CodeGen/SelectionDAG/TargetLowering.cpp
5559 ↗(On Diff #187172)

It might be tidier to do:

} else {
  if (VT.isVector())
    return DAG.UnrollVectorOverflowOp(Node);
  ...
}
nikic updated this revision to Diff 187268.Feb 18 2019, 12:32 PM

Fix shift amount type handling, use ExtractVectorElements, change location of vector check.

nikic marked 4 inline comments as done.Feb 18 2019, 12:37 PM
nikic added inline comments.
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
981 ↗(On Diff #187172)

This is a somewhat odd case: The existing code here was actually correct, and my change wasn't. The problem is that we're still during legalization and the integer sizes involved may have more bits than can fit into the shift amount type, so just using it can lead to asserts if odd types like i300 are used (I added a testcase). I'm now using the getShiftAmountTyForConstant() helper to properly handle this.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
8963 ↗(On Diff #187172)

How can append() be used in this context?

RKSimon added inline comments.Feb 18 2019, 12:43 PM
lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
981 ↗(On Diff #187172)

SGTM

nikic updated this revision to Diff 187395.Feb 19 2019, 9:40 AM

Rebase over rL354359.

RKSimon added inline comments.Feb 19 2019, 10:23 AM
lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1228 ↗(On Diff #187395)

Don't use auto

1230 ↗(On Diff #187395)

Would it be better to perform the unroll in this function instead of TLI.expandMULO? This matches existing behaviour

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
8960 ↗(On Diff #187395)

ResScalars.append(ResNE - NE, getUNDEF(ResEltVT));
OvScalars.append(ResNE - NE, getUNDEF(OvEltVT));

lib/CodeGen/SelectionDAG/TargetLowering.cpp
5526 ↗(On Diff #187395)

I still reckon you'd be better off returning a bool and the result/overflow SDValues by reference args - similar to what we do for the majority of the TargetLowering::expand* functions

nikic updated this revision to Diff 187446.Feb 19 2019, 2:37 PM

Change expandMULO signature to write results by reference. Move unrolling into vec op legalization. Use append.

RKSimon accepted this revision.Feb 20 2019, 2:47 AM

LGTM

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
8953 ↗(On Diff #187446)

for (unsigned i = 0; i < NE; ++i) {

This revision is now accepted and ready to land.Feb 20 2019, 2:47 AM
This revision was automatically updated to reflect the committed changes.
llvm/trunk/test/CodeGen/AArch64/vec_umulo.ll