Page MenuHomePhabricator

DAGcombiner: Handle correctly non-splat power of 2 -1 divisor

Authored by RKSimon on Apr 19 2018, 1:26 AM.



The combine added in commit 329525 overlooked the case where
one, but not all, of the divisor elements is -1.
-1 is the only power of two value for which the sdiv expansion recipe

Fixes pr37119

Diff Detail


Event Timeline

zvi created this revision.Apr 19 2018, 1:26 AM
zvi updated this revision to Diff 143057.Apr 19 2018, 1:37 AM

Minor improvement: No need for the select if there are no negatives

dyung added a subscriber: dyung.Apr 23 2018, 3:43 PM
RKSimon added inline comments.Apr 25 2018, 7:03 AM
2949 ↗(On Diff #143057)

Do you need the getSplatBuildVector? I think DAG.getAllOnesConstant can accept a vector type:

DAG.getAllOnesConstant(DL, VT)

@zvi Any movement on this? I can commandeer if you're still busy.

@zvi Any movement on this? I can commandeer if you're still busy.

@zvi I'm going to commandeer this and get it finished if you don't mind.

RKSimon commandeered this revision.Jun 19 2018, 7:33 AM
RKSimon edited reviewers, added: zvi; removed: RKSimon.

Commandeering as @zvi is busy with other things

RKSimon updated this revision to Diff 153068.Jun 27 2018, 6:12 AM
RKSimon added a reviewer: efriedma.

Refactored the patch to blend the special case values in at the end of the pow2 calculations.

I'll add the SignedMin case in a followup but I wanted to keep this patch focussed on the -1 divisor case.

zvi added a comment.Jun 27 2018, 8:39 AM

Sorry for not being responsive. Won't have time to work on this, so thanks @RKSimon for taking charge.

efriedma added inline comments.Jun 27 2018, 11:29 AM
3047 ↗(On Diff #153068)

Not really relevant to this review, but I think KnownNegatives is dead.

3104 ↗(On Diff #153068)

Instead of separate special cases for 1 and -1, could you move the special case for 1 before the result is negated? Would probably result in a little less code for vectors like <1, -1, 2, -2>

3108 ↗(On Diff #153068)

I'm not sure you actually need a special case for MIN_SIGNED; I think r335720 solved the issue.

RKSimon marked an inline comment as done.Jun 28 2018, 3:59 AM
RKSimon added inline comments.
3104 ↗(On Diff #153068)

I'll look at this next

3108 ↗(On Diff #153068)

I think you're right - I'll leave the IsPowerOfTwo early out in for now but won't need to add a special case selection.

RKSimon updated this revision to Diff 153307.Jun 28 2018, 6:27 AM

Merged the 2 negations together as suggested by @efriedma

I think you can save a blend by emitting one select for both "1" and "-1"? (SDValue IsOneOrAllOnes = DAG.getNode(ISD::OR, VT, IsAllOnes, IsOne); etc.)

RKSimon updated this revision to Diff 153456.Jun 29 2018, 2:52 AM

Nice catch! Merged -1/+1 blending into single op.

This revision is now accepted and ready to land.Jun 29 2018, 1:47 PM
This revision was automatically updated to reflect the committed changes.