Page MenuHomePhabricator

[RISCV] Support insertion of misaligned subvectors
ClosedPublic

Authored by frasercrmck on Thu, Feb 18, 9:40 AM.

Details

Summary

This patch extends the support for RVV INSERT_SUBVECTOR to cover those
which don't align to a vector register boundary. Like the support for
EXTRACT_SUBVECTOR in D96959, it accomplishes this by extracting the
nearest register-sized subvector (a subregister operation), then sliding
the vector down with VSLIDEDOWN, inserting the subvector to the first
position, and sliding the vector back up again afterwards.

Unlike subvector extraction, for vectors that occupy less than a full
vector register we must preserve the untouched elements. We do this by
lowering to an LMUL=1 INSERT_SUBVECTOR using the above method and
lowering that to a VSLIDEUP with a zero offset. This uses a
tail-undisturbed policy and so has the effect of "sliding in" the
subvector elements while preserving the surrounding ones.

Diff Detail

Event Timeline

frasercrmck created this revision.Thu, Feb 18, 9:40 AM
frasercrmck requested review of this revision.Thu, Feb 18, 9:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Feb 18, 9:40 AM
craig.topper added inline comments.Fri, Feb 19, 12:03 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
966

Should this really be a tail undisturbed vmv.v.v?

Can a less than full vector insert be done with a single slideup if you calculate the offset and limit vl to mark the end of the range?

Can a less than full vector insert be done with a single slideup if you calculate the offset and limit vl to mark the end of the range?

Probably. That occurred to me slightly too late yesterday and I was going to take a look at that this morning. It would also handle i1 vectors in the same way as others as you're forced to limit VL to account for (some of) those.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
966

Yeah, that's what I was searching for originally. I was hesitant to introduce a new pseudo for that purpose in this patch, though. It might be mostly irrelevant if I optimize this to use vslideup everywhere. Though the vmv.v.v would still provide a potential optimization for correctly-sized types at offset 0. Do you think that's worth it?

  • rebase
  • add float vectors
  • lower simply VSLIDEUP of LMUL=1 type (no VSLIDEDOWNs)
  • silence unused variable warning
  • test inserting into undef subvectors

Sadly this regresses some of the fixed-length tests. Because INSERT_SUBVECTOR is now "custom" (not "legal") it doesn't fold bitcasts around the operation:

Vector/type-legalized selection DAG: %bb.0 'mulhs_vx_v2i64:'
                t27: v4i32 = BUILD_VECTOR Constant:i32<63>, Constant:i32<0>, Constant:i32<63>, Constant:i32<0>
              t28: v2i64 = bitcast t27
            t40: nxv1i64 = insert_subvector undef:nxv1i64, t28, Constant:i32<0>

it used to perform this dag-combine:

Optimized vector-legalized selection DAG: %bb.0 'mulhs_vx_v2i64:'
                t27: v4i32 = BUILD_VECTOR Constant:i32<63>, Constant:i32<0>, Constant:i32<63>, Constant:i32<0>
              t46: nxv2i32 = insert_subvector undef:nxv2i32, t27, Constant:i32<0>
            t47: nxv1i64 = bitcast t46

which for scalable vectors is a no-op, but for fixed-length vectors it still goes through the stack.

Sounds like there's a sort-of dependency on supporting fixed-length bitcasts before we can merge this. @craig.topper, I don't suppose you're looking into it?

craig.topper accepted this revision.Mon, Feb 22, 11:58 AM

LGTM other than that one question

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2254

Should this be unsigned instead of auto? I think getConstantOperandVal returns uint64_t but the rest of this code uses unsigned. I probably should have asked this question in the EXTRACT_SUBVECTOR patch.

This revision is now accepted and ready to land.Mon, Feb 22, 11:58 AM
frasercrmck marked an inline comment as done.Tue, Feb 23, 1:26 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2254

Yes, sounds good. Thanks for spotting that. The EXTRACT_SUBVECTOR code is using unsigned.

frasercrmck marked an inline comment as done.
  • rebase
  • address review feedback
This revision was landed with ongoing or failed builds.Tue, Feb 23, 2:37 AM
This revision was automatically updated to reflect the committed changes.