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

[luke957]

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").

[craig.topper]

Test case?

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h

58

Doesn't getMinRVVVectorSizeInBits() already max with 128?

[luke957]

Add test case riscv-unroll.ll

[luke957]

In D97549#2590428, @craig.topper wrote:

Test case?

It does.

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h

58

It does.

[frasercrmck]

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]

In D97549#2596672, @frasercrmck wrote:

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.

[HsiangKai]

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]

llvm/test/Transforms/LoopVectorize/RISCV/riscv-unroll.ll

1 ↗

(On Diff #327340)

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

[frasercrmck]

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.

[luke957]

llvm/test/Transforms/LoopVectorize/RISCV/riscv-unroll.ll

1 ↗

(On Diff #328025)

Add RUN line for riscv32.

[luke957]

In D97549#2602700, @frasercrmck wrote:

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

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

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h

   bool shouldExpandReduction(const IntrinsicInst *II) const;
   bool supportsScalableVectors() const { return ST->hasStdExtV(); }
   Optional<unsigned> getMaxVScale() const;
+
+  unsigned getRegisterBitWidth(bool Vector) const {
+    if (Vector) {
+      if (ST->hasStdExtV())
+        return std::max(ST->getMinRVVVectorSizeInBits(), 128u);
+      return 0;
+    }
+    return ST->getXLen();
+  }
+
 };
 
 } // end namespace llvm