This is an archive of the discontinued LLVM Phabricator instance.

[ARM] add, or, and and xor with shl combining
ClosedPublic

Authored by samparker on Sep 20 2017, 8:45 AM.

Details

Summary

The generic dag combiner will fold:

(shl (add x, c1), c2) -> (add (shl x, c2), c1 << c2)
(shl (or x, c1), c2) -> (or (shl x, c2), c1 << c2)

This can create constants which are too large to use as an immediate. Many ALU operations are also able of performing the shl, so we can unfold the transformation to prevent a mov imm instruction from being generated.

Other patterns, such as b + ((a << 1) | 510), can also be simplified in the same manner.

Diff Detail

Event Timeline

samparker created this revision.Sep 20 2017, 8:45 AM
john.brawn edited edge metadata.Oct 17 2017, 4:20 AM

This patch does three distinct things (swap CMP operands, separate out OR->BFI conversion, unfold shl) and I think it would make more sense to therefore do three separate patches.

lib/Target/ARM/ARMISelLowering.cpp
3858–3861

This could be simplified using getShiftOpcForNode from ARMSelectionDAGInfo.h (check LHS is not no_shift, check RHS is no_shift).

3862

Not in 16-bit thumb, though it's probably harmless to swap them in that case. The comment should mention that though.

10269

Moving this comment looks a little odd.

10327

This should be called something else as there's already a PerformBFICombine and the name here violates the rule that the argument to PerformXCombine is a node of type X (here it's an OR of an AND). Probably PerformORCombineToBFI.

Thanks for the comments John. I'll make the changes and split the patch.

cheers,
sam

samparker updated this revision to Diff 119339.Oct 17 2017, 8:53 AM
samparker edited the summary of this revision. (Show Details)

Updated now that BFI combine and the compare operand swapping have been extracted into their own patches.

john.brawn added inline comments.Oct 18 2017, 5:47 AM
lib/Target/ARM/ARMISelLowering.cpp
10101

'Unfold' is probably the wrong description of this as we may get these kind of instruction sequences without any folding having happened e.g.

int fn(int a, int b)
{
  return b + ((a << 1) | 510);
}
10104–10105

Could do with a comment here saying something like "no 16-bit thumb instructions with shifted operand".

10107–10108

We should be doing this for XOR and AND as well.

Thanks again John, I'll make the changes.

samparker updated this revision to Diff 119572.Oct 19 2017, 5:05 AM
samparker retitled this revision from [ARM] add, or, shl combining to [ARM] add, or, and and xor with shl combining.
samparker edited the summary of this revision. (Show Details)

Added support for simplifying XOR and AND nodes and added tests for them, this meant having to modify a few tests too. Also changed and added some comments and renamed the function.

john.brawn added inline comments.Oct 24 2017, 9:38 AM
lib/Target/ARM/ARMISelLowering.cpp
10164

You're setting BinOp here then immediately setting it to something else a few lines later, you should be setting it just once.

10165–10178

It would make more sense to do this check nearer to the start of the function. Also you could check that all uses don't already have a shifted operand, as in that case doing this transform doesn't have any benefit. That should mean you don't have to adjust the load-combine tests.

samparker updated this revision to Diff 120217.Oct 25 2017, 2:34 AM

Thanks John, I've made those changes.

This revision is now accepted and ready to land.Nov 1 2017, 10:50 AM
This revision was automatically updated to reflect the committed changes.