Page MenuHomePhabricator

[TargetLowering][DAGCombine][MSP430] Shift Amount Threshold in DAGCombine (2/2)
ClosedPublic

Authored by joanlluch on Oct 17 2019, 10:35 AM.

Details

Summary

Continuation of D69116

Contributes to a Fix for https://bugs.llvm.org/show_bug.cgi?id=43559

Should be commited after D69099 and D69116

Using the TLI hook in DAGCombine.cpp.

Diff Detail

Event Timeline

joanlluch created this revision.Oct 17 2019, 10:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2019, 10:35 AM
joanlluch updated this revision to Diff 225594.Oct 18 2019, 4:02 AM

Made sure the updated diff file accounts for recent trunk changes

joanlluch updated this revision to Diff 226021.Oct 22 2019, 4:19 AM

Code style correction

spatel added inline comments.Oct 28 2019, 5:17 AM
llvm/test/CodeGen/MSP430/shift-amount-threshold.ll
55–56

Do we have test coverage for each code diff?
It would be good to add a code comment for each test that describes the transform that we are trying to avoid.
For example, this one could copy the formula from the corresponding source code comment:

// sext i1 (setgt iN X, -1) --> sra (not X), (N - 1)
spatel added inline comments.Oct 28 2019, 5:25 AM
llvm/test/CodeGen/MSP430/shift-amount-threshold.ll
55–56

Also, the IR does not correspond exactly to that comment. It's fine to include tests with 'select', but if there is an intermediate transform that turns it into 'sext' or 'zext', then we should have a test that begins with that pattern in IR too if possible. We want to make sure that we do not accidentally bypass these changes with future patches.

joanlluch updated this revision to Diff 226960.Oct 29 2019, 2:23 PM

Updated tests to reflect each covered case in an independent way with more focused IR patterns.
Added comments to the test file that refer to the transformation we are dealing with.

joanlluch marked 4 inline comments as done.Oct 29 2019, 2:40 PM
joanlluch added inline comments.
llvm/test/CodeGen/MSP430/shift-amount-threshold.ll
55–56

I updated the test file as per your suggestion. Test cases are now focused on the individual transformations that we are trying to avoid. I also changed the IR to avoid intermediate transformations and focus on the pattern of interest.
Unfortunately, these changes implied some depart from the 'baseline' tests that were posted before, and thus the diff appears now a bit mixed, but I think it is still informative enough to see the improvements.

joanlluch planned changes to this revision.Oct 29 2019, 9:36 PM
joanlluch marked 2 inline comments as not done.
joanlluch added inline comments.
llvm/test/CodeGen/MSP430/shift-amount-threshold.ll
55–56

Ok, please forget for now about this diff. I will insert a new ‘baseline’ tests commit so that at least the test diffs here shows up hopefully cleaner.

joanlluch marked an inline comment as not done.Oct 30 2019, 1:57 AM

I created a new baseline revision, D69609, that help to better reflect the effects of applying this patch. The referred revision should therefore be applied before this one for consistency reasons.

joanlluch updated this revision to Diff 227048.Oct 30 2019, 2:05 AM

@spatel I updated this revision diff according to your suggestions. I also created a new baseline test revision to help identify the effects of this patch. I hope this is ok. Thanks in advance for your time.

@asl I identified you as the creator of the MSP430 patch that optimised 8 bit and larger shifts by combining them with bswap and extend instructions. This creates relatively cheap instruction sequences for exactly 8 bit shifts.

I am considering replacing my current implementation of getShiftAmountThreshold, by a version that passes the intended shift amount as a parameter and returns a boolean. For example, let's say that we have

bool shouldAvoidTransformToShift(EVT VT, unsigned amount) const override;

Which would return false by default.

For the MSP430 it could be implemented like this

bool MSP430TargetLowering::shouldAvoidTransformToShift(EVT VT, unsigned amount) const {
  return !(amount == 8 || amount<=2);
}

Would this make sense for the MSP430?

Thanks.

joanlluch marked 3 inline comments as done.Oct 30 2019, 3:42 AM
asl added a comment.Oct 30 2019, 11:19 AM

@joanlluch Yes, having the version with amount does make sense as target may know how to efficiently lower some of the shifts with crazy tricks. And in the view of this patch it's easier to preserve the sequences rather than match and undo later. I guess for MSP430 we'd need to allow 9 as possible "special" shift value, as select is lowered to 3 instructions (test + branch + mov of some kind) and removing branch overall has some additional value like allowing more code folding / reducing register pressure.

joanlluch updated this revision to Diff 227313.EditedOct 31 2019, 10:57 AM

Updated diff to account for the latest trunk changes

@asl Ok thanks, I agree that 9 bit shifts may also benefit from the version with amount.
If this is ok, I will propose the TLI function replacement on a different patch after this is reviewed, to keep things organised.

spatel accepted this revision.Nov 4 2019, 8:23 AM

LGTM

llvm/test/CodeGen/MSP430/shift-amount-threshold.ll
135

Looks like the spelling fix was not updated here. You have to run the script again to regenerate the CHECK lines if you change the test source code.

This revision is now accepted and ready to land.Nov 4 2019, 8:23 AM
This revision was automatically updated to reflect the committed changes.