This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Enable fixed-length vectorization of LoopVectorizer for RISC-V Vector
ClosedPublic

Authored by luke957 on Feb 26 2021, 6:03 AM.

Details

Summary

By implementing the method "unsigned RISCVTTIImpl::getRegisterBitWidth(bool Vector)", fixed-length vectorization is enabled when possible.
Without this method, the "#pragma clang loop" directive is needed to enable vectorization(or the cost model may inform LLVM that "Vectorization is possible but not beneficial").

Diff Detail

Event Timeline

luke957 created this revision.Feb 26 2021, 6:03 AM
luke957 requested review of this revision.Feb 26 2021, 6:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2021, 6:03 AM

Test case?

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h
58

Doesn't getMinRVVVectorSizeInBits() already max with 128?

luke957 updated this revision to Diff 327340.Mar 1 2021, 7:13 PM

Add test case riscv-unroll.ll

Test case?

It does.

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h
58

It does.

The changes LGTM, thanks for adding a test case.

However, I think the commit title/message could be expanded upon somewhat. For example, *how* does it improve LoopVectorizer support? Since we've got both fixed-length and scalable vectorization, I think it would be helpful to clarify that this relates to fixed-length vectorization. More specifically, doesn't this change *enable* fixed-length vectorization, not just improve upon it?

luke957 retitled this revision from [RISCV] Improve support of LoopVectorizer for RISC-V Vector to [RISCV] Enable fixed-length vectorization of LoopVectorizer for RISC-V Vector.Mar 2 2021, 7:17 PM
luke957 edited the summary of this revision. (Show Details)
luke957 marked an inline comment as not done.Mar 2 2021, 7:24 PM

The changes LGTM, thanks for adding a test case.

However, I think the commit title/message could be expanded upon somewhat. For example, *how* does it improve LoopVectorizer support? Since we've got both fixed-length and scalable vectorization, I think it would be helpful to clarify that this relates to fixed-length vectorization. More specifically, doesn't this change *enable* fixed-length vectorization, not just improve upon it?

Thanks for the review and advice. I have rewritten the title and message.

xmj added a subscriber: xmj.Mar 2 2021, 7:56 PM
HsiangKai added inline comments.Mar 3 2021, 6:33 PM
llvm/test/Transforms/LoopVectorize/RISCV/riscv-unroll.ll
1 ↗(On Diff #327340)

-mattr=+experimental-v should be enough.

Should we specify -riscv-v-vector-bits-max in this test case?

luke957 updated this revision to Diff 328025.Mar 3 2021, 10:42 PM
luke957 added inline comments.Mar 3 2021, 10:46 PM
llvm/test/Transforms/LoopVectorize/RISCV/riscv-unroll.ll
1 ↗(On Diff #327340)

Yeah, "-riscv-v-vector-bits-max" is not needed.

frasercrmck accepted this revision.Mar 4 2021, 1:32 AM

LGTM. I wouldn't say my last suggestion is a hard requirement.

llvm/test/Transforms/LoopVectorize/RISCV/riscv-unroll.ll
1 ↗(On Diff #328025)

We could perhaps add an extra RUN line for riscv32 just to make sure nothing silly is going on.

This revision is now accepted and ready to land.Mar 4 2021, 1:32 AM
luke957 updated this revision to Diff 328123.Mar 4 2021, 4:23 AM
luke957 added inline comments.Mar 4 2021, 4:26 AM
llvm/test/Transforms/LoopVectorize/RISCV/riscv-unroll.ll
1 ↗(On Diff #328025)

Add RUN line for riscv32.

LGTM. I wouldn't say my last suggestion is a hard requirement.

Yeah, it would be better to add RUN line for riscv32.