Page MenuHomePhabricator

[SelectionDAG]Reduce masked data movement chains and memory access widths

Authored by dnsampaio on May 11 2018, 5:27 AM.



Final goal:
detect that, in an i16 data type example:
i16 M[x] = (M[x] & 0xFF00) | (M[x] >> 8 & 0xFF)
can be reduced to an i8 data movement:
i8 M[x] = i8 M[x+1]
The existing code does not handle shifted mask, nor truncating stores and sext loads.

Step 1: Width reduction masked loads by shifted masks
the operation
and i16 (load i16, [M]), 0xFF00
can be replaced by
shl i16 (load i8, [M+1], zext i16), 8

Diff Detail

Event Timeline

dnsampaio created this revision.May 11 2018, 5:27 AM

Please update your diff file in order to contain all the file content. Thanks.

dnsampaio retitled this revision from Reduce masked data movement chains and memory access widths to [SelectionDAG]Reduce masked data movement chains and memory access widths.May 11 2018, 5:36 AM
dnsampaio updated this revision to Diff 146310.May 11 2018, 5:41 AM

Update diff to contain all the file contents.

Hi Diogo,

This looks like two patches to me so please separate them. I would also expect to see more tests, this is quite a large change so try to cover some of the corner cases.

Also, one styling comment is that we use capital case for variables.



N is an AND, right? This would be easier to read if just N is passed, which you can then add an assert on the opcode, and then you can make N0 and N1 local variables.


unsigned is fine, otherwise try to avoid using C-style casting. It would also be more readable if it was called something like 'ShiftOpcode'.


Don't rely on the ordering of these values. How about using the switch below to default to returning instead of an unreachable?


Pull the cast out of the loop, the nullptr check is also unnecessary.


Is SRA really legal here?


Best to just have a TODO comment and remove these opcodes from the switch.


You've already created a local var for 'shiftAmount', it would be easier to follow if you continued to use it.


Don't think you need to add these as they're not new.


AndC could be a nullptr here.


This looks like two bits of new functionality, so please separate them into two patches.

20 ↗(On Diff #146310)

Please remove the attributes and metadata.

dnsampaio updated this revision to Diff 149747.Jun 4 2018, 6:59 AM
dnsampaio marked 11 inline comments as done.
dnsampaio edited the summary of this revision. (Show Details)

Attended all review requests, including spitting into 2 patches.
Fixed the cases where the SRA can be accepted.
Added more unit-tests.

rogfer01 added inline comments.

Typo here in Redudant?

dnsampaio updated this revision to Diff 150280.Jun 7 2018, 2:10 AM
dnsampaio marked an inline comment as done.

Fixed typo.

Hi Diogo,

You seem to have forgot to re-add your tests now that you've split the patch.

dnsampaio updated this revision to Diff 150319.Jun 7 2018, 6:42 AM

Re-inserted tests.

Hi Diogo,

Thanks for adding the tests again, but they're still not exercising your change properly - I don't see how your testing the SHL, ROTL or ROTR paths. Also by not checking the input and output registers, using regex or otherwise, you're not testing that the data flow is correct.
This patch should also be split again: into the shift folding and the load reduction, dag combining can be a pain so its best to break into down as much as possible.





Is this needed for the rest of the patch to work? I would have expected that once the above transformation has been performed, that the transform you've added below would later be performed by the existing code in ReduceLoadWidth.

dnsampaio updated this revision to Diff 151670.Jun 18 2018, 5:24 AM
dnsampaio marked 2 inline comments as done.
dnsampaio edited the summary of this revision. (Show Details)

Spitted the patch into 3 parts, and added further tests.

Hi Diogo,

This is looking good, I have just a couple of inline queries. Also, please add a couple of negative tests that use a mask that isn't a multiple of 8 bits.



OR instead of AND?


Could you explain what you're checking here?


remove or uncomment.


Could you comment on why both of these are being set please?


Is it now possible to do a ldrh with an offset of 1 by using a mask of 0xff0?

samparker added inline comments.Jun 19 2018, 2:55 AM

i meant a mask of 0xffff00...

dnsampaio abandoned this revision.Dec 28 2018, 9:26 AM
dnsampaio marked 6 inline comments as done.