Page MenuHomePhabricator

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

Authored by joanlluch on Nov 9 2019, 2:47 AM.

Details

Summary

Replaces

unsigned getShiftAmountThreshold(EVT VT)

by

bool shouldAvoidTransformToShift(EVT VT, unsigned amount)

thus giving more flexibility for targets to decide whether particular shift amounts must be considered expensive or not.

Updates the MSP430 target with a custom implementation.

This continues D69116, D69120, D69326 and updates them, so all of them must be committed before this.

Existing tests apply, a few more have been added.

Diff Detail

Event Timeline

joanlluch created this revision.Nov 9 2019, 2:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2019, 2:47 AM
spatel added inline comments.Nov 10 2019, 6:50 AM
llvm/include/llvm/CodeGen/TargetLowering.h
2611–2614
/// Return true if creating a shift of the type by the given 
/// amount is not profitable.
2613
llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
364

This changes the MSP430 output for shift by 8 or 9? If yes, there should be tests that show this difference.

joanlluch marked an inline comment as done.Nov 11 2019, 7:29 AM
joanlluch added inline comments.
llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
364

I commited baseline tests to show this (D70083). I did not wait for review in this particular case, as I understand it's simple enough. (It does not modify any code) (I hope this is ok).
I will submit an updated patch for this.

joanlluch updated this revision to Diff 228700.Nov 11 2019, 8:05 AM
joanlluch marked 4 inline comments as done.Nov 11 2019, 8:20 AM
joanlluch added inline comments.
llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
364

After all the related previous patches, showing improvements for the special case of 8 and 9 bit shifts on the MSP430 is really borderline. But they can be found in a couple of cases. The intention of this series of patches is to provide additional hooks for targets that may benefit from them. In the particular case of the MSP430, it's possibly debatable whether 8 and 9 bit shifts should be systematically factored-in, but in the general case, I think that it is good that the TLI hook semantics provide more flexibility to targets. Please let me know how it looks now. Thanks

joanlluch edited the summary of this revision. (Show Details)Nov 11 2019, 9:59 AM
spatel accepted this revision.Nov 12 2019, 2:37 PM

LGTM - I'm assuming less code is better on this target.
(if @asl or someone else can confirm, that would be great)

This revision is now accepted and ready to land.Nov 12 2019, 2:37 PM
This revision was automatically updated to reflect the committed changes.