This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Combine add + shl to alsl.[w/d/wu]
ClosedPublic

Authored by gonglingqin on Aug 25 2022, 11:20 PM.

Diff Detail

Event Timeline

gonglingqin created this revision.Aug 25 2022, 11:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2022, 11:20 PM
gonglingqin requested review of this revision.Aug 25 2022, 11:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2022, 11:20 PM

For the patch title: not all shifted additions can be expressed with alsl.* due to the limited imm range, so we could say Combine eligible add + shl ... to better convey the meaning.

Also you could add some more tests covering other possible choices of the imm, and one negative test case showing non-powers-of-2 multipliers are not covered. It may or may not be complete (that would probably cause the testcase number to explode), just some coverage would be nice.

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
737

"Shifted addition"?

745

This feels a bit weird, could a more generic node be used here instead of the rather concrete bstrpick one?

For the patch title: not all shifted additions can be expressed with alsl.* due to the limited imm range, so we could say Combine eligible add + shl ... to better convey the meaning.

Also you could add some more tests covering other possible choices of the imm, and one negative test case showing non-powers-of-2 multipliers are not covered. It may or may not be complete (that would probably cause the testcase number to explode), just some coverage would be nice.

Thanks, I will modify the title of the patch and add testcases.

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
737

Thanks, I will change it.

745

Since zero_extend will be converted to and, and the and node generates LoongArchISD::BSTRPICK by combining before the instruction selection process. Therefore, if common nodes are used for matching, optimization needs to be added during the Combine process, which requires more code. So tblgen was chosen to simplify the code.

Address @xen0n's comments.

xen0n added a comment.Aug 26 2022, 1:25 AM

I mean, it should be possible to test alsl.* xx, xx, xx, N where N is 1, 2 or 4 right? Currently only 3 is being tested.

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
745

Okay that explains things. Thanks.

I mean, it should be possible to test alsl.* xx, xx, xx, N where N is 1, 2 or 4 right? Currently only 3 is being tested.

Thanks, I will add these testcases

Add testcases. Address @xen0n's comments.

xen0n accepted this revision.Aug 26 2022, 2:43 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Aug 26 2022, 2:43 AM
SixWeining accepted this revision.Aug 27 2022, 4:26 AM
This revision was landed with ongoing or failed builds.Aug 28 2022, 7:12 PM
This revision was automatically updated to reflect the committed changes.