Page MenuHomePhabricator

[PowerPC] Collapse RLDICL/RLDICR into RLDIC when possible
ClosedPublic

Authored by nemanjai on Apr 8 2019, 5:26 AM.

Details

Summary

Generally speaking, we lower to an optimal rotate sequence for nodes visible in the SDAG. However, there are instances where the two rotates are not visible at ISEL time - most notably those in a very common sequence when lowering switch statements to jump tables.
A common situation is a switch on a 32-bit integer. This value has to have the upper 32 bits cleared and because jump table offsets are word offsets, the value needs to be shifted left by 2 bits. We currently emit the clear and the left shift as two separate instructions, but this is not needed as we can lower it to a single RLDIC.
This patch just cleans that up.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Apr 8 2019, 5:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2019, 5:26 AM
Herald added a subscriber: kbarton. · View Herald Transcript
nemanjai updated this revision to Diff 194142.Apr 8 2019, 7:27 AM

Cleaned up the comments and the MB parameter for RLDIC does not need to include the shift that was originally on the RLDICL.

stefanp requested changes to this revision.Apr 12 2019, 7:43 AM

The patch does what we want it to do in most cases (including the example that motivated it) but some boundary conditions need to be considered before it is safe in all cases.

lib/Target/PowerPC/PPCMIPeephole.cpp
51

nit:
collapsed

785

The value for NewMB can be negative.
For example:

%1:g8rc = RLDICL %0, 10, 1
%2:g8rc = RLDICR %1, 10, 43

In this case NewMB = -9
I changed the testcase you provided and used the above two lines and I get an assert.

We may still be able to handle the negative case (because this is a rotate) but it may require another if statement and special handling.

793

The value for (63 - NewSH) can also be negative and I think it needs to be a special case.
Each shift has to be less than or equal to 63 but the sum of two shifts can be greater than 63.

test/CodeGen/PowerPC/collapse-rotates.mir
54

Had to delete this line on the machine I was running on to get this test to pass...
The error is:

error: YAML:53:23: unknown key 'machineFunctionInfo'
machineFunctionInfo: {}

I'm not sure what the issue is here but I feel like that line is not universally supported.

This revision now requires changes to proceed.Apr 12 2019, 7:43 AM
nemanjai marked 3 inline comments as done.May 2 2019, 4:39 AM
nemanjai added inline comments.
lib/Target/PowerPC/PPCMIPeephole.cpp
51

Good catch, thanks.

793

These are very good points. I will add a check to bail out if either NewMB or NewSH are larger than 63.

test/CodeGen/PowerPC/collapse-rotates.mir
54

I am not sure what that's about. Perhaps you were on an older revision where that wasn't part of MIR?

nemanjai updated this revision to Diff 197748.May 2 2019, 4:40 AM
nemanjai marked 3 inline comments as done.

Added a check for wrapping in the computation of the new shift amount and mask start bit.

stefanp accepted this revision.May 2 2019, 10:49 AM

LGTM.
The one very minor comment I have I think can be addressed on commit.

test/CodeGen/PowerPC/collapse-rotates.mir
54

I have tried to reproduce this since but I have not been able to. I would say don't worry about this.
I assume the issue was on my end.

test/CodeGen/PowerPC/jump-tables-collapse-rotate.ll
3

nit:
This is something that I find helps with readability of the test case.
Is it possible to add: -ppc-asm-full-reg-names

This revision is now accepted and ready to land.May 2 2019, 10:49 AM
This revision was automatically updated to reflect the committed changes.