[RISCV] vp.merge of nxv16i1 tries to be unrolled

[lukel97]

After #101641, this vp merge began to be emitted from the loop vectorizer with EVL tail folding from SPEC CPU 2017:

define <vscale x 16 x i1> @f(<vscale x 16 x i1> %m, <vscale x 16 x i1> %x, <vscale x 16 x i1> %y, i32 %evl) {
  %1 = tail call <vscale x 16 x i1> @llvm.vp.merge.nxv16i1(<vscale x 16 x i1> %m, <vscale x 16 x i1> %x, <vscale x 16 x i1> %y, i32 %evl)
  ret <vscale x 16 x i1> %1
}

It crashes with llc -mtriple=riscv64 -mattr=+v with LLVM ERROR: Invalid size request on a scalable vector. because it tries to unroll it when legalizing vector ops. The DAG after type legalization looks like:

Type-legalized selection DAG: %bb.0 'f:'
SelectionDAG has 15 nodes:
  t0: ch,glue = EntryToken
      t2: nxv16i1,ch = CopyFromReg t0, Register:nxv16i1 %0
      t4: nxv16i1,ch = CopyFromReg t0, Register:nxv16i1 %1
      t6: nxv16i1,ch = CopyFromReg t0, Register:nxv16i1 %2
        t8: i64,ch = CopyFromReg t0, Register:i64 %3
      t18: i64 = and t8, Constant:i64<4294967295>
    t12: nxv16i1 = vp_merge t2, t4, t6, t18
  t15: ch,glue = CopyToReg t0, Register:nxv16i1 $v0, t12
  t16: ch = RISCVISD::RET_GLUE t15, Register:nxv16i1 $v0, t15:1

[llvmbot]

@llvm/issue-subscribers-backend-risc-v

Author: Luke Lau (lukel97)

After #101641, this vp merge began to be emitted from the loop vectorizer with EVL tail folding from SPEC CPU 2017:

define &lt;vscale x 16 x i1&gt; @<!-- -->f(&lt;vscale x 16 x i1&gt; %m, &lt;vscale x 16 x i1&gt; %x, &lt;vscale x 16 x i1&gt; %y, i32 %evl) {
  %1 = tail call &lt;vscale x 16 x i1&gt; @<!-- -->llvm.vp.merge.nxv16i1(&lt;vscale x 16 x i1&gt; %m, &lt;vscale x 16 x i1&gt; %x, &lt;vscale x 16 x i1&gt; %y, i32 %evl)
  ret &lt;vscale x 16 x i1&gt; %1
}

It crashes with llc -mtriple=riscv64 -mattr=+v with LLVM ERROR: Invalid size request on a scalable vector. because it tries to unroll it when legalizing vector ops. The DAG after type legalization looks like:

Type-legalized selection DAG: %bb.0 'f:'
SelectionDAG has 15 nodes:
  t0: ch,glue = EntryToken
      t2: nxv16i1,ch = CopyFromReg t0, Register:nxv16i1 %0
      t4: nxv16i1,ch = CopyFromReg t0, Register:nxv16i1 %1
      t6: nxv16i1,ch = CopyFromReg t0, Register:nxv16i1 %2
        t8: i64,ch = CopyFromReg t0, Register:i64 %3
      t18: i64 = and t8, Constant:i64<4294967295>
    t12: nxv16i1 = vp_merge t2, t4, t6, t18
  t15: ch,glue = CopyToReg t0, Register:nxv16i1 $v0, t12
  t16: ch = RISCVISD::RET_GLUE t15, Register:nxv16i1 $v0, t15:1

[fhahn]

cc @Mel-Chen

[lukel97]

I think we should be able to lower this in the RISC-V backend, and I think it makes sense to transform selects to VP nodes during EVL tail folding since it allows us to get rid of a mask user.

But I think this goes to show that VP intrinsics aren't as well supported as regular, non-predicated instructions in the RISC-V backend.

Now that the VLOptimizer pass is enabled by default, do we want to reconsider emitting VP intrinsics during EVL tail folding if they're not necessary? I.e. not selects/loads/stores, but binary ops/other intrinsics.

My gut feeling is that we'll be missing out on a lot of combines and optimisations that just aren't implemented for VP intrinsics/nodes.

In that case we should probably emit regular instructions where possible in the loop vectorizer, then teach DAGCombiner and friends to handle VP nodes first. Then we could revisit emitting VP intrinsics once the codegen is a bit better.

(This is somewhat speculation: It would be good to evaluate this first! But to do that we need to fix the assertion failures 🥲)

cc @Mel-Chen @arcbbb @LiqinWeng @preames @npanchen @michaelmaitland

[topperc]

We probably have a fix for this in our downstream. We've been using VP intrinsics in our vectorizer for a while. I'll see if I can find it.

[fhahn]

How can we get such issues surfaced upstream sooner? It sounds like it might be good to put a hold on future EVL patches in LoopVectorize until all issues with the current state are fixed.

[preames]

How can we get such issues surfaced upstream sooner? It sounds like it might be good to put a hold on future EVL patches in LoopVectorize until all issues with the current state are fixed.

Well, at least some of the current EVL patches are addressing functional issues, so pausing that would seem slightly self defeating. However, I do agree that we need to get the EVL stuff into a correct working state before continuing to work on optimization quality. This does seem to be happening, but it needs to be more of a focus. @lukel97's recent test runs (and reporting) is a great help here, but we clearly need to be doing more of this.

[fhahn]

Well, at least some of the current EVL patches are addressing functional issues, so pausing that would seem slightly self defeating. However, I do agree that we need to get the EVL stuff into a correct working state before continuing to work on optimization quality. This does seem to be happening, but it needs to be more of a focus. @lukel97's recent test runs (and reporting) is a great help here, but we clearly need to be doing more of this.

Yep I didn't mean any patches addressing issues, only patches generating more vp.* intrinsics

[Mel-Chen]

Apologize for not noticing the issue with i1 vp.merge in codegen.

This appears to be the i1 vp.merge generated by AnyOf.
While isLegalToVectorizeReduction does check isLegalElementTypeForRVV to filter out cases where the type is i1, AnyOf is later transformed into an i1 Or reduction.
The quickest way to address this would be to disable AnyOf in isLegalToVectorizeReduction. If we still want to enable AnyOf reduction, one option is to attempt implementing an in-loop AnyOf, and another is to try widening the reduction type to the smallest legal element type. @fhahn Do you have a preferred approach?

How can we get such issues surfaced upstream sooner?

I believe the reason this issue wasn't discovered earlier is that AnyOf lacks test coverage in the llvm test-suite. I’ve strengthened this in llvm/llvm-test-suite#195 — please take a look.

[lukel97]

The quickest way to address this would be to disable AnyOf in isLegalToVectorizeReduction

After #120479 everything is working again, does it still need disabled? I understand that i1 vectors aren't supported in isLegalElementTypeForRVV, but presumably the loop vectorizer emits them in other places too e.g. predication or mask tail folding. So it doesn't sound too unreasonable to expect the backend to support them.