Page MenuHomePhabricator

[DAGCombiner] Improve shift by select of constant
ClosedPublic

Authored by laytonio on Oct 28 2020, 3:53 PM.

Details

Summary

Clean up a TODO, to support folding a shift of a constant by a select of constants, on targets with different shift operand sizes.

Diff Detail

Event Timeline

laytonio created this revision.Oct 28 2020, 3:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2020, 3:53 PM
laytonio requested review of this revision.Oct 28 2020, 3:53 PM
RKSimon added inline comments.Oct 29 2020, 5:07 AM
llvm/test/CodeGen/PowerPC/select_const.ll
630

any ideas on these regressions?

laytonio added inline comments.Oct 29 2020, 6:06 AM
llvm/test/CodeGen/PowerPC/select_const.ll
630

any ideas on these regressions?

Forgive me, I'm not very familiar with PowerPC, but is this actually worse? It now looks just like the above unaltered test for select of constants shifted by constant. If it is worse, should we change that as well?

RKSimon added inline comments.
llvm/test/CodeGen/PowerPC/select_const.ll
630

@nemanjai how much of an issue is this?

lebedev.ri added inline comments.Dec 1 2020, 1:07 AM
llvm/test/CodeGen/PowerPC/select_const.ll
630
RKSimon added a reviewer: amyk.Dec 9 2020, 4:02 AM
RKSimon added a subscriber: amyk.
RKSimon added inline comments.
llvm/test/CodeGen/PowerPC/select_const.ll
630

@amyk Any comments?

qiucf added a reviewer: Restricted Project.Dec 16 2020, 11:40 PM
steven.zhang added inline comments.
llvm/test/CodeGen/PowerPC/select_const.ll
630

It looks good to me as before the change, clrlwi, subfic and slw are chained together, while the new one only chain two instructions.(andi. and iselgt) And they didn't have too much latency difference AFAIK. But please correct me if I miss something here.

lkail added a subscriber: lkail.Dec 17 2020, 12:05 AM
lkail added inline comments.
llvm/test/CodeGen/PowerPC/select_const.ll
630

Agree, my pfm test shows that using isel here is better.

Looks reasonable to me then.

RKSimon accepted this revision.Dec 17 2020, 2:42 AM

LGTM

This revision is now accepted and ready to land.Dec 17 2020, 2:42 AM
nemanjai accepted this revision.Dec 17 2020, 2:54 AM

I am really sorry that I've missed these notifications. The PPC changes are perfectly fine. This might lead to regressions if the shift amount is large enough to require more than a single instruction to produce each constant (i.e. the constant no longer fits in a 16-bit signed immediate). But there's work underway to improve constant materialization in the back end that will alleviate this problem.

So LGTM and sorry about the delay.

amyk accepted this revision.Dec 17 2020, 6:49 AM

I'm really sorry for the delay. LGTM to me, as well.

Thank you everyone for looking at this! Could one of you please commit this for me? Layton Kifer <laytonkifer@gmail.com>

This revision was landed with ongoing or failed builds.Dec 17 2020, 6:24 PM
This revision was automatically updated to reflect the committed changes.