This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Optimise ASRL/LSRL to smaller shifts using demand bits.
ClosedPublic

Authored by dmgreen on Feb 28 2020, 10:41 AM.

Details

Summary

The ASRL/LSRL long shifts are generated from 64bit shifts. Once we have them, it might turn out that enough of the 64bit result was not required that we can use a smaller shift to perform the same result. As the smaller shift can in general be folded in more way, such as into add instructions in one of the test cases here, we can use the demand bit analysis to prefer the smaller shifts where we can.

Diff Detail

Event Timeline

dmgreen created this revision.Feb 28 2020, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2020, 10:41 AM

Are there tests for shifts greater than 31? And don't we do some weird stuff with negative shifts, or is that just with custom nodes?

llvm/lib/Target/ARM/ARMISelLowering.h
358 ↗(On Diff #247305)

Doesn't this need override?

This revision was not accepted when it landed; it landed in state Needs Review.Mar 4 2020, 10:41 AM
This revision was automatically updated to reflect the committed changes.
dmgreen reopened this revision.Mar 4 2020, 10:48 AM

Whoops. That was the wrong number on that commit.

dmgreen updated this revision to Diff 248256.Mar 4 2020, 11:04 AM

Rebase to on top of D75553 (I had not really considered them together when I wrote that, only from the times we would generate the nodes from shifts. From intrinsics more cases can come up). It makes sense to do that one first as this has become more complex.

I have tried to add a few of the obvious cases that I thought might come up, when parts of the shift are not used. The intrinsics are really designed for variable shifts but they can become constant during optimisation (and the first example I saw of someone using them was asrl(X, 23), as opposed to using a standard shift).

This doesn't feel like the prettiest code I've ever written. Suggestions welcome.

samparker added inline comments.Mar 5 2020, 1:23 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
14124 ↗(On Diff #248256)

For readability, maybe introduce a lambda to help create the shift and do the replacement? Some aptly named variables for shift ranges and whether we're doing a logical/left/right shift could also help.

dmgreen updated this revision to Diff 248714.Mar 6 2020, 6:20 AM

I decided that this was really doing two separate things, and should split it back in two. This it the first (not so ugly) part for the time being. I will try to get to the second part later.

samparker accepted this revision.Mar 12 2020, 1:58 AM

Simplified the simplify! LGTM

This revision is now accepted and ready to land.Mar 12 2020, 1:58 AM
This revision was automatically updated to reflect the committed changes.