This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombine] Improve (sra (sra x, c1), c2) -> (sra x, (add c1, c2)) folding
ClosedPublic

Authored by RKSimon on Jul 21 2017, 5:42 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Jul 21 2017, 5:42 AM
andrew.zhogin edited edge metadata.Jul 24 2017, 1:04 AM

Isn't it a little regression for scalar types?

N1.getValueType() == N0.getOperand(1).getValueType()

Instead of

zeroExtendToMatch(c1, c2, 1 /* Overflow Bit */);

There is no vector analog of zeroExtendToMatch currently, right?

Btw, I don't think I have rights to accept the revision.

RKSimon updated this revision to Diff 160079.Aug 10 2018, 3:02 AM

Resurrecting this old patch, refactored to correctly handle overflows of the sum of shifts.

spatel added inline comments.Aug 16 2018, 9:34 AM
test/CodeGen/X86/combine-sra.ll
133–134

To provide better coverage, add (or adjust this) test where both of the component shifts are below bitwidth, but the sum exceeds bitwidth?

RKSimon added inline comments.Aug 16 2018, 10:24 AM
test/CodeGen/X86/combine-sra.ll
133–134

See @combine_vec_ashr_ashr2 above

spatel accepted this revision.Aug 16 2018, 12:28 PM

LGTM. I notice we don't fold the non-splat case of this pattern in IR. Do you think it's worth adding there too, or backend is good enough?

This revision is now accepted and ready to land.Aug 16 2018, 12:28 PM

LGTM. I notice we don't fold the non-splat case of this pattern in IR. Do you think it's worth adding there too, or backend is good enough?

Add it if you can, properly supporting vectors is always a good idea ;-)

Is there the equivalent logical shifts -> zero combine?

LGTM. I notice we don't fold the non-splat case of this pattern in IR. Do you think it's worth adding there too, or backend is good enough?

Add it if you can, properly supporting vectors is always a good idea ;-)

Is there the equivalent logical shifts -> zero combine?

AFAIK, no. I think splats are well supported now in instcombine, but we haven't generalized many transforms for non-splats.

This revision was automatically updated to reflect the committed changes.