This is an archive of the discontinued LLVM Phabricator instance.

[SDAG][RISCV] Avoid expanding is-power-of-2 pattern on riscv32/64 with zbb
ClosedPublic

Authored by dtcxzyw on Jul 26 2023, 9:01 PM.

Details

Summary

This patch adjusts the legality check for riscv to use cpop/cpopw since isOperationLegal(ISD::CTPOP, MVT::i32) returns false on rv64gc_zbb.
Clang vs gcc: https://godbolt.org/z/rc3s4hjPh

Diff Detail

Event Timeline

dtcxzyw created this revision.Jul 26 2023, 9:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2023, 9:01 PM
dtcxzyw requested review of this revision.Jul 26 2023, 9:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2023, 9:01 PM
craig.topper added inline comments.Jul 26 2023, 9:23 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
18590

The vector part of this isn't tested.

dtcxzyw updated this revision to Diff 544630.Jul 27 2023, 12:32 AM
dtcxzyw edited the summary of this revision. (Show Details)

Add missing vector tests.

dtcxzyw marked an inline comment as done.Jul 27 2023, 12:36 AM
dtcxzyw updated this revision to Diff 544654.Jul 27 2023, 1:52 AM

Tune instseq for ctpop_i32/64_ugt_one.

luke added inline comments.Jul 27 2023, 5:26 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
18590

I think if we check that the operation is legal then we don’t need to check for zbb/zvbb, since it’s only legal with those extensions

dtcxzyw added inline comments.Jul 27 2023, 7:12 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
18590

I am not familiar with the riscv vector crypto extension. I just do the same check as https://github.com/llvm/llvm-project/blob/3c4d2221de0e9e7b259a308aab7e72e3f2f926f1/llvm/lib/Target/RISCV/RISCVISelLowering.cpp#L764.

It is worth noting that isOperationLegal(ISD::CTPOP, MVT::i32) returns false on rv64gc_zbb. We will miss some optimizations if we only check the legality of ctpop.

craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
18590

@luke957 it won't be "legal" for fixed vectors since we will need to Custom lower to a RISCVISD node with VL.

RKSimon resigned from this revision.Aug 23 2023, 8:26 AM
craig.topper added inline comments.Aug 24 2023, 9:46 AM
llvm/test/CodeGen/RISCV/rvv/ctpop-sdnode.ll
860

These tests already pass.

dtcxzyw added inline comments.Aug 25 2023, 9:03 AM
llvm/test/CodeGen/RISCV/rvv/ctpop-sdnode.ll
860

The vector part of this isn't tested.

Should I remove these useless tests? We always emit vcpop for the vector part.

craig.topper added inline comments.Aug 25 2023, 9:31 AM
llvm/test/CodeGen/RISCV/rvv/ctpop-sdnode.ll
860

You can keep them. Just be clear in the description that they already pass.

We do need additional tests for fixed vector without "vscale x". I bet they don't pass without this patch.

dtcxzyw updated this revision to Diff 553748.Aug 26 2023, 10:55 AM
dtcxzyw removed a reviewer: RKSimon.
  • Rebase
  • Add fixed-length vector tests
dtcxzyw marked 4 inline comments as done.Aug 26 2023, 10:56 AM
craig.topper added inline comments.Aug 28 2023, 11:23 AM
llvm/test/CodeGen/RISCV/rv64zbb.ll
700

I meant with V and the Zvbb extensions enabled so we have vector cpop instructions.

dtcxzyw updated this revision to Diff 554271.Aug 29 2023, 5:35 AM
  • Rebase
  • Add fixed-length vector tests with +v,+zvbb
dtcxzyw marked an inline comment as done.Aug 29 2023, 5:36 AM
dtcxzyw added inline comments.Aug 29 2023, 8:35 PM
llvm/test/CodeGen/RISCV/rvv/ctpop-sdnode.ll
1366

These fixed-vector tests should be moved into fixed-vectors-ctpop.ll.

dtcxzyw updated this revision to Diff 554635.Aug 30 2023, 2:02 AM
  • Rebase
  • Move fixed-length vector tests into fixed-vectors-ctpop.ll
This revision is now accepted and ready to land.Sep 16 2023, 9:32 AM
This revision was landed with ongoing or failed builds.Sep 16 2023, 11:56 AM
This revision was automatically updated to reflect the committed changes.