Page MenuHomePhabricator

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

Authored by joanlluch on Oct 17 2019, 9:51 AM.



This is a continuation of D68982, following community recommendations

Provides a TLI hook to allow targets to relax the emission of shifts, thus enabling codegen improvements on targets with no multiple shift instructions and cheap selects or branches.

Contributes to a Fix for

Should be commited after D69099

This is part 1, defining the TLI hook, incorporating it in the MSP430 target, and using it in TagetLogwering.cpp.

Diff Detail

Event Timeline

joanlluch created this revision.Oct 17 2019, 9:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 17 2019, 9:51 AM
spatel added inline comments.Oct 17 2019, 12:38 PM

It may not be used with vector types currently, but it would be safer to use:


This is probably not working as you expected because the shift type may not be the same as the value type.
I added an x86 test to demonstrate:


Here and above the indentation looks non-standard. Use "clang-format" to correct.

joanlluch added inline comments.Oct 17 2019, 2:44 PM



Would passing the shift value (not the constant amount type) to getShiftAmountThreshold work in all cases?


I didn't know about clang-format. I see what you mean now. I will correct that.

joanlluch added inline comments.Oct 17 2019, 3:01 PM

I mean passing the shift value type, not the shift value, of course., so in the case above would be TLI.getShiftAmountThreshold(N0.getValueType())
Would that look good?

joanlluch updated this revision to Diff 225587.Oct 18 2019, 3:36 AM

Updated with latest suggestions

joanlluch marked 3 inline comments as done.Oct 18 2019, 3:37 AM
spatel accepted this revision.Oct 18 2019, 12:21 PM

LGTM - see inline for a minor improvement.

Do you have commit access?


Within this block alone, we call N0.getValueType() at least 6 times. Better to make that a local variable like:

EVT N0VT = N0.getValueType();
This revision is now accepted and ready to land.Oct 18 2019, 12:21 PM
joanlluch marked an inline comment as done.Oct 18 2019, 3:21 PM
joanlluch added inline comments.

Totally agree!, but I was a bit shy of touching code made by others.
Will do

LGTM - see inline for a minor improvement.

Do you have commit access?

Thanks for accepting this.
I don't have commit access. I would appreciate that you push it for me after I apply your last suggested improvement.

joanlluch updated this revision to Diff 225744.Oct 19 2019, 1:33 AM
joanlluch marked an inline comment as not done.

Move repeated calls to N0.getValueType() into a variable

joanlluch marked an inline comment as done.Oct 19 2019, 1:34 AM
This revision was automatically updated to reflect the committed changes.
asl added a comment.Oct 19 2019, 10:02 AM

LGTM, thanks!