Page MenuHomePhabricator

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

Authored by dnsampaio on Jun 4 2018, 10:55 AM.

Details

Summary

Detects patterns that copies the higher half of an element to its lower half.
Example, it replaces the pattern:
x= (zext i8 ( load 1 [M+1] ) to i16)
x2= i16 or(x, shl(x, 8))
store 1 (truncate i16 x2 to i8) [M]

by:
x = i8 load 1 [M+1]
store 1 i8 x [M]

Diff Detail

Event Timeline

dnsampaio created this revision.Jun 4 2018, 10:55 AM
javed.absar added inline comments.Jun 4 2018, 2:08 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13109

I think the comment here can be improved. e.g. 'load 1 from M' is confusing. You probably mean load i8 from memory?

Also, it looks like you are combing two points into one statement which may confuse readers. Better to simplify it.

13115

Would be better to return nullptr instead of 'llvm_unreachable' in case its not ISD::OR

rogfer01 added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13114–13115

If you choose to leave the assertion, consider simplifying it into something like

assert(OR.getOpcode() == ISD::OR && "Expecting ISD::OR");

See the recommendation described in http://llvm.org/docs/ProgrammersManual.html#programmatic-errors

13176–13181

Was this left accidentally? If not, perhaps a single TODO comment describing what can be done next will be clearer.

dnsampaio updated this revision to Diff 150281.Jun 7 2018, 2:13 AM
dnsampaio marked 4 inline comments as done.

Implemented one TODO left on the code, and added check to verify if the load is sign extend, when the load width is smaller than the store width. Else the result of the OR operation would not be known.

samparker added inline comments.Jun 8 2018, 2:12 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13112

Can this not be implemented in, or as a helper function for, ReduceLoadOpStoreWidth? That function is already performing safety checks and handling the load and store creation correctly. It looks like it needs help understanding a shifted operand to the OR, instead of just a constant.

lib/Transforms/Utils/SimplifyIndVar.cpp
787 ↗(On Diff #150281)

Why these changes?

dnsampaio retitled this revision from [SelectionDAG]Reduce masked data movement chains and memory access widths pt2 to [SelectionDAG]Reduce masked data movement chains and memory access widths pt3.Jun 18 2018, 5:28 AM
dnsampaio updated this revision to Diff 151701.Jun 18 2018, 5:35 AM
dnsampaio marked 2 inline comments as done.

Added further tests.

Hi Diogo,

I will look at this again once the two parent changes have been committed, as I'm not familiar with these functions. I'm confused as to why there isn't a ReduceStoreWidth to pair with ReduceLoadWidth. These LoadOpStore functions have a lot of overlap and related functionality that I suspect can be refactored nicely.

cheers,
sam

I am not suggesting that I will ask you to refactor by the way! I just want to spend some more time looking at these functions and having a play with your other two patches.

niyaroje accepted this revision.Jul 1 2018, 3:45 PM
This revision is now accepted and ready to land.Jul 1 2018, 3:45 PM
niyaroje requested changes to this revision.Jul 1 2018, 10:32 PM
This revision now requires changes to proceed.Jul 1 2018, 10:32 PM
niyaroje accepted this revision.Jul 1 2018, 10:33 PM
This revision is now accepted and ready to land.Jul 1 2018, 10:33 PM
dnsampaio abandoned this revision.Dec 28 2018, 9:25 AM