Skip to content

Conversation

@stevesuzuki-arm
Copy link
Contributor

No description provided.

}

Value *CodeGen_LLVM::reverse_vector(llvm::Value *vec) {
if (effective_vscale > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this will affect RISC V as well and thus needs to be validated there. Is there test coverage for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correctness_predicated_store_load
correctness_vector_math
cover this route at least on aarch64 host.
Unfortunately, I don't have a setup to test RISCV host. It would be good if it is tested on CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made it ARM only for now. So, I suppose RISC V should not be affected.

Copy link
Member

@zvookin zvookin Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if you don't mind, please go ahead and put it back to being generic. I tested with and without the original change to generate code for RISC V and while the llvm.vector.reverse intrinsic shows up in the LLVM IR with the PR, and doea not without it, there is no difference in the generated RISC V assembly. (And it does compile correctly, at least with recent LLVM.) Of course I only looked at a simple test case and didn't run anything on real hardware, but I think this is enough diligence to say this change is a good one.

I'll go ahead and approve and merge after the change back, or another comment on this if you'd rather not for some reason.

LLVM VectorReverse could be used commonly,
but for now we use it only in CodeGen_ARM until tested.
This reverts commit 7fd00b6
because the original change has been tested with RISCV target
according to the PR comments.
@alexreinking
Copy link
Member

Failures are unrelated (WebGPU API nonsense and a failing arm32 LLVM build)

@alexreinking alexreinking merged commit 2e52451 into halide:main Dec 6, 2025
15 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants