This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Add codegen support for ISD::CTPOP, ISD::CTTZ and ISD::CTLZ
ClosedPublic

Authored by gonglingqin on Aug 10 2022, 1:36 AM.

Diff Detail

Event Timeline

gonglingqin created this revision.Aug 10 2022, 1:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 1:36 AM
gonglingqin requested review of this revision.Aug 10 2022, 1:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 1:36 AM
xen0n added inline comments.Aug 10 2022, 1:52 AM
llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
401

switch (NumOp)?

llvm/lib/Target/LoongArch/LoongArchISelLowering.h
47

nit: Bit counting

49

Other *.w nodes are named *_W, so add an underscore for consistency?

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
692–695

no need to guard with Predicates = [IsLA32]?

gonglingqin added inline comments.Aug 10 2022, 2:29 AM
llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
401

Thanks, I will change that.

llvm/lib/Target/LoongArch/LoongArchISelLowering.h
47

Thanks, I will change that.

49

Thanks, I will add an underscore.

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
692–695

It needs to guard with Predicates = [IsLA32], I will fix it, thanks.

Address @xen0n's comments.

xen0n added a comment.Aug 10 2022, 3:41 AM

Seems good to me.

P.S. If this lands after D131512 you have to refresh your testcases. But I'd happily rebase if that patch lands after this one too.

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
680–687

Okay so after you grouped the LA32 bit-counting ops together it may be better to move this block below, and add a // Bit counting operations to describe the two groups of operations. Adding these patterns near the multiplications just for saving an if Predicates = [IsLA64] block doesn't help with readability.

Seems good to me.

P.S. If this lands after D131512 you have to refresh your testcases. But I'd happily rebase if that patch lands after this one too.

Thanks for your reminding. I will refresh my testcases after D131512 landed.

llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
680–687

Thanks, I will change that.

Address @xen0n's comments.
Modify the location of LA64 bit-counting Ops and add comments to increase readability.

Seems good to me.

P.S. If this lands after D131512 you have to refresh your testcases. But I'd happily rebase if that patch lands after this one too.

Thanks for your reminding. I will refresh my testcases after D131512 landed.

D131512 has landed. You can rebase now.

This revision is now accepted and ready to land.Aug 10 2022, 8:33 PM
xen0n accepted this revision.Aug 10 2022, 10:09 PM
This revision was landed with ongoing or failed builds.Aug 11 2022, 11:15 PM
This revision was automatically updated to reflect the committed changes.