Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SLP/RISCV: add test for vectorized ctpop, like in X86 #65330

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

artagnon
Copy link
Contributor

@artagnon artagnon commented Sep 5, 2023

Recently, 7f26c27 turned on SLP by default for RISC-V, and although
there are quite a few tests for SLP under the X86/ target, it is unclear
whether the same constructs would be vectorized on RISC-V. This patch
takes a step in the direction of remedying this, by noticing that ctpop
is often vectorized on RISC-V, and adding four tests for different
integer widths.

@alexey-bataev
Copy link
Member

Would be good to add the corresponding test for the analysis/cost model

@preames
Copy link
Collaborator

preames commented Sep 5, 2023

Please rework your commit message. If your goal is to exercise the costs, then you really should be writing a cost model test, not an SLP test. I suspect that despite your wording, your actual intention is to add coverage showing that SLP can vectorize these examples on RISCV. (The difference in wording there is that the cost of the ctpop expansion is only part of that.)

Note that we do appear to have cost model tests already in test/Analysis/CostModel/RISCV/int-bit-manip.ll.

@artagnon
Copy link
Contributor Author

artagnon commented Sep 5, 2023

Thanks @preames; re-worded.

@eopXD
Copy link
Member

eopXD commented Sep 6, 2023

Why specifically ctpop. There are much more coverage for the X86 backend, is the whole plan to add them commit by commit to the RISC-V backend?

@artagnon
Copy link
Contributor Author

artagnon commented Sep 7, 2023

Why specifically ctpop.

I was investigating some benchmarks where I came across ctpop being vectorized by gcc-aarch64, but not llvm-riscv, so I was curious about whether LLVM does any vectorization of ctpop at all on RISC-V. This patch is a result of that investigation; nothing more.

There are much more coverage for the X86 backend, is the whole plan to add them commit by commit to the RISC-V backend?

Do you have a different suggestion?

Recently, 7f26c27 turned on SLP by default for RISC-V, and although
there are quite a few tests for SLP under the X86/ target, it is unclear
whether the same constructs would be vectorized on RISC-V. This patch
takes a step in the direction of remedying this, by noticing that ctpop
is often vectorized on RISC-V, and adding four tests for different
integer widths.
@preames
Copy link
Collaborator

preames commented Sep 7, 2023

LGTM as well.

For your follow up on this, note that vector ctpop exists only under ZVBB. I suspect we didn't update the costs for ctpop if zvbb is enabled. Without it, vectorizing is probably not profitable with the default expansion. That'd be the first thing I'd check. If my guess is right, you're also want a CostModel test.

@lukel97 FYI

@artagnon artagnon merged commit 7f49957 into llvm:main Sep 7, 2023
2 checks passed
@artagnon artagnon deleted the slp-ctpop-riscv branch September 7, 2023 16:02
avillega pushed a commit to avillega/llvm-project that referenced this pull request Sep 11, 2023
Recently, 7f26c27 turned on SLP by default for RISC-V, and although
there are quite a few tests for SLP under the X86/ target, it is unclear
whether the same constructs would be vectorized on RISC-V. This patch
takes a step in the direction of remedying this, by noticing that ctpop
is often vectorized on RISC-V, and adding four tests for different
integer widths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants