This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Improved bitfield instruction selection.
ClosedPublic

Authored by gberry on Sep 16 2015, 10:39 AM.

Details

Summary

For bitfield insert OR matching, check both operands for larger pattern
first before checking for smaller pattern.

Add pattern for unsigned bitfield insert-in-zero done with SHL+AND.

Resolves PR21631.

Diff Detail

Event Timeline

gberry updated this revision to Diff 34904.Sep 16 2015, 10:39 AM
gberry retitled this revision from to [AArch64] Improved bitfield instruction selection..
gberry updated this object.
gberry added reviewers: jmolloy, t.p.northover.
gberry added subscribers: mcrosier, llvm-commits.
jmolloy requested changes to this revision.Sep 17 2015, 1:24 AM
jmolloy edited edge metadata.

Hi Geoff,

Thanks for working on this. I have some comments below.

Cheers,

James

lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
1924

This is called from 3 places after your patch. 2 of them explicitly set BiggerPattern.

It would make sense to remove the default from this parameter and just set it in the last callsite. This would also allow you to put the output MaskWidth at the end of the argument list.

1964

The code here appears to contradict the comment - which is correct?

2004–2019

Please use a capital for induction variables. I is normal.

2006

I now understand what's going on here after some grepping and grokking, but this comment wasn't as helpful as it could have been. My understanding is:

There are situations where both isBitfieldExtractOp(A, B, true) and isBitfieldExtractOp(B, A, false) match. The latter should give better results.

Perhaps expanding a bit more in that comment would ease understanding?

Also, please capitalize first letters of sentences and end with a full stop (and spell out "operand" in full).

2110

This isn't a particularly useful comment.

2120

"Op0" would be more in keeping with the rest of LLVM.

2126

A comment here explaining what the immediate values are and why would be useful.

This revision now requires changes to proceed.Sep 17 2015, 1:24 AM
gberry updated this revision to Diff 35020.Sep 17 2015, 11:33 AM
gberry edited edge metadata.

Update to address comments from James Molloy

James, thanks for the comments. I've added a new revision which should hopefully address all of them.

jmolloy accepted this revision.Sep 18 2015, 8:48 AM
jmolloy edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Sep 18 2015, 8:48 AM
This revision was automatically updated to reflect the committed changes.