[RISCV] Add a test showing an incorrect vsetvli insertion

[frasercrmck]

Summary

This patch adds a reduced test case which identifies an illegal vsetvli
inserted by the compiler. The compiler emits a vsetvli which is intended
to preserve VL with the SEW/LMUL ratio e32/m1 when in fact the VL could
have been set by e64/m2 in a predecessor block.

[frasercrmck]

I haven't found the time to investigate the actual cause of this bug yet, so I thought I'd stick it up as a regression test so that people are aware of the issue; since it's something the simulator chokes on it may or may not actually produce a failure depending on the specific simulator/test case. I can maybe take a look later this week.

[craig.topper]

Why is preserving vl wrong here? Isn't the vl 4 regardless of which it came from?

I believe the pass is preserving it because it believes that AVL=4 will produce the same vl as long as sew/lmul is constant which it should be for sew=64/lmul=2 vs sew=32/lmul=1

[frasercrmck]

In D106286#2888143, @craig.topper wrote:

Why is preserving vl wrong here? Isn't the vl 4 regardless of which it came from?

I believe the pass is preserving it because it believes that AVL=4 will produce the same vl as long as sew/lmul is constant which it should be for sew=64/lmul=2 vs sew=32/lmul=1

You're right, I must have reduced the test case too far at one point and stopped thinking properly. I'll go back to the original failing case.

[frasercrmck]

• reduce the test correctly

[frasercrmck]

Sorry about that @craig.topper I was rushing it. The test case is now reduced a lot better. I believe it's an issue where the vmv.x.s "doesn't need" an AVL (it's RISCV::NoRegister) because the operation is VL-agnostic, as it were, so we fall into a trap where we insert a vsetvli zero,zero even though that's an illegal attempt to preserve VL.

[craig.topper]

That's very interesting and I think it has always been broken. I modified the test case to not depend on VLS support and it does this on 12.0.1 as well.

define i32 @foo(<vscale x 2 x i32> %a, <vscale x 4 x i64> %x, <vscale x 4 x i64>* %y) {
  %index = add <vscale x 4 x i64> %x, %x
  store <vscale x 4 x i64> %index, <vscale x 4 x i64>* %y
  %elt = extractelement <vscale x 2 x i32> %a, i64 0
  ret i32 %elt
}

We don't have a VL to use here and I don't think we should make one up. I think the right fix is to add the VL argument to the intrinsic and the instruction.

[craig.topper]

llvm/test/CodeGen/RISCV/rvv/vsetvli-regression.ll

2

Can we use

define i32 @foo(<vscale x 2 x i32> %a, <vscale x 4 x i64> %x, <vscale x 4 x i64>* %y) {
  %index = add <vscale x 4 x i64> %x, %x
  store <vscale x 4 x i64> %index, <vscale x 4 x i64>* %y
  %elt = extractelement <vscale x 2 x i32> %a, i64 0
  ret i32 %elt
}

to avoid the mix of fixed and scalable.

[frasercrmck]

• update test according to feedback

[frasercrmck]

• remove unused vlen-bits-min

[frasercrmck]

In D106286#2890831, @craig.topper wrote:

That's very interesting and I think it has always been broken. [...[
We don't have a VL to use here and I don't think we should make one up. I think the right fix is to add the VL argument to the intrinsic and the instruction.

Interesting, thanks for the context. I'll keep discussion about the fix(es) to D106403 to keep everything well-threaded.

llvm/test/CodeGen/RISCV/rvv/vsetvli-regression.ll

2

Sure. The original test was a mix but I think it's best to keep it as simple as possible.

[frasercrmck]

@craig.topper do you think we should merge this in or just abandon it since D106403 presumably covers the same case?

[craig.topper]

LGTM I'll go ahead and merge this.

llvm/test/CodeGen/RISCV/rvv/vsetvli-regression.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=riscv64 -mattr=+experimental-v | FileCheck %s
+
+; This test checks a regression in the vsetvli insertion pass. We used to
+; prserve the VL on the second vsetvli with ratio e32/m1, when the the last
+; update of VL was the vsetvli with e64/m4. Changing VTYPE here changes VLMAX
+; which may make the original VL invalid. Instead of preserving it we use 0.
+
+define i32 @illegal_preserve_vl(<vscale x 2 x i32> %a, <vscale x 4 x i64> %x, <vscale x 4 x i64>* %y) {
+; CHECK-LABEL: illegal_preserve_vl:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    vsetvli a1, zero, e64, m4, ta, mu
+; CHECK-NEXT:    vadd.vv v28, v12, v12
+; CHECK-NEXT:    vs4r.v v28, (a0)
+; CHECK-NEXT:    vsetivli zero, 0, e32, m1, ta, mu
+; CHECK-NEXT:    vmv.x.s a0, v8
+; CHECK-NEXT:    ret
+  %index = add <vscale x 4 x i64> %x, %x
+  store <vscale x 4 x i64> %index, <vscale x 4 x i64>* %y
+  %elt = extractelement <vscale x 2 x i32> %a, i64 0
+  ret i32 %elt
+}