Page MenuHomePhabricator

[SDAG] Support vector UMULO/SMULO

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



Second part of

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


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
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.

8953 ↗(On Diff #187172)

Use ExtractVectorElements ?

8963 ↗(On Diff #187172)

Use append()?

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.
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.

8963 ↗(On Diff #187172)

How can append() be used in this context?

RKSimon added inline comments.Feb 18 2019, 12:43 PM
981 ↗(On Diff #187172)


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
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

8960 ↗(On Diff #187395)

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

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


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.