Page MenuHomePhabricator

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

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

Details

Summary

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 https://bugs.llvm.org/show_bug.cgi?id=43559

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.Thu, Oct 17, 9:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Oct 17, 9:51 AM
spatel added inline comments.Thu, Oct 17, 12:38 PM
llvm/include/llvm/CodeGen/TargetLowering.h
2614

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

VT.getScalarSizeInBits()
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3621

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:
rL375158

3632–3634

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

joanlluch added inline comments.Thu, Oct 17, 2:44 PM
llvm/include/llvm/CodeGen/TargetLowering.h
2614

okey

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3621

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

3632–3634

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

joanlluch added inline comments.Thu, Oct 17, 3:01 PM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3621

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.Fri, Oct 18, 3:36 AM

Updated with latest suggestions

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

LGTM - see inline for a minor improvement.

Do you have commit access?

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3611–3612

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.Fri, Oct 18, 12:21 PM
joanlluch marked an inline comment as done.Fri, Oct 18, 3:21 PM
joanlluch added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
3611–3612

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.Sat, Oct 19, 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.Sat, Oct 19, 1:34 AM
This revision was automatically updated to reflect the committed changes.
asl added a comment.Sat, Oct 19, 10:02 AM

LGTM, thanks!