[GISEL] Add G_INSERT_SUBVECTOR and G_EXTRACT_SUBVECTOR #84538
Merged
+267
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-llvm-globalisel Author: Michael Maitland (michaelmaitland) ChangesG_INSERT and G_EXTRACT are not sufficient to use to represent both INSERT/EXTRACT on a subregister and INSERT/EXTRACT on a vector. We would like to be able to INSERT/EXTRACT on vectors in cases that INSERT/EXTRACT on vector subregisters are not sufficient, so we add these opcodes. I tried to do a patch where we treated G_EXTRACT as both G_EXTRACT_SUBVECTOR and G_EXTRACT_SUBREG, but ran into an infinite loop at this point in the SDAG equivalent code. Full diff: https://github.com/llvm/llvm-project/pull/84538.diff 8 Files Affected:
diff --git a/llvm/docs/GlobalISel/GenericOpcode.rst b/llvm/docs/GlobalISel/GenericOpcode.rst
index dda367607d0432..f9f9e1186460ee 100644
--- a/llvm/docs/GlobalISel/GenericOpcode.rst
+++ b/llvm/docs/GlobalISel/GenericOpcode.rst
@@ -607,6 +607,41 @@ See the LLVM LangRef entry on '``llvm.lround.*'`` for details on behaviour.
Vector Specific Operations
--------------------------
+G_INSERT_SUBVECTOR
+^^^^^^^^^^^^^^^^^^
+
+Insert the second source vector into the first source vector. The index operand
+represents the starting index in the first source vector at which the second
+source vector should be inserted into.
+
+The index must be a constant multiple of the second source vector's minimum
+vector length. If the vectors are scalable, then the index is first scaled by
+the runtime scaling factor. The indices inserted in the source vector must be
+valid indicies of that vector. If this condition cannot be determined statically
+but is false at runtime, then the result vector is undefined.
+
+.. code-block:: none
+
+ %2:_(<vscale x 4 x i64>) = G_INSERT_SUBVECTOR %0:_(<vscale x 4 x i64>), %1:_(<vscale x 2 x i64>), 0
+
+G_EXTRACT_SUBVECTOR
+^^^^^^^^^^^^^^^^^^^
+
+Extract a vector of destination type from the source vector. The index operand
+represents the starting index from which a subvector is extracted from
+the source vector.
+
+The index must be a constant multiple of the source vector's minimum vector
+length. If the source vector is a scalable vector, then the index is first
+scaled by the runtime scaling factor. The indices extracted from the source
+vector must be valid indicies of that vector. If this condition cannot be
+determined statically but is false at runtime, then the result vector is
+undefined.
+
+.. code-block:: none
+
+ %3:_(<vscale x 4 x i64>) = G_EXTRACT_SUBVECTOR %2:_(<vscale x 8 x i64>), 2
+
G_CONCAT_VECTORS
^^^^^^^^^^^^^^^^
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index 6762b1b360d5e8..4732eaf4ee27c5 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -1121,6 +1121,25 @@ class MachineIRBuilder {
MachineInstrBuilder buildConcatVectors(const DstOp &Res,
ArrayRef<Register> Ops);
+ /// Build and insert `Res = G_INSERT_SUBVECTOR Src0, Src1, Idx`.
+ ///
+ /// \pre setBasicBlock or setMI must have been called.
+ /// \pre \p Res, \p Src0, and \p Src1 must be generic virtual registers with
+ /// vector type.
+ ///
+ /// \return a MachineInstrBuilder for the newly created instruction.
+ MachineInstrBuilder buildInsertSubvector(const DstOp &Res, const SrcOp &Src0,
+ const SrcOp &Src1, unsigned Index);
+
+ /// Build and insert `Res = G_EXTRACT_SUBVECTOR Src, Idx0`.
+ ///
+ /// \pre setBasicBlock or setMI must have been called.
+ /// \pre \p Res and \p Src must be generic virtual registers with vector type.
+ ///
+ /// \return a MachineInstrBuilder for the newly created instruction.
+ MachineInstrBuilder buildExtractSubvector(const DstOp &Res, const SrcOp &Src,
+ unsigned Index);
+
MachineInstrBuilder buildInsert(const DstOp &Res, const SrcOp &Src,
const SrcOp &Op, unsigned Index);
diff --git a/llvm/include/llvm/Support/TargetOpcodes.def b/llvm/include/llvm/Support/TargetOpcodes.def
index 94fba491148b2e..3dade14f043b60 100644
--- a/llvm/include/llvm/Support/TargetOpcodes.def
+++ b/llvm/include/llvm/Support/TargetOpcodes.def
@@ -727,6 +727,12 @@ HANDLE_TARGET_OPCODE(G_BR)
/// Generic branch to jump table entry.
HANDLE_TARGET_OPCODE(G_BRJT)
+/// Generic insert subvector.
+HANDLE_TARGET_OPCODE(G_INSERT_SUBVECTOR)
+
+/// Generic extract subvector.
+HANDLE_TARGET_OPCODE(G_EXTRACT_SUBVECTOR)
+
/// Generic insertelement.
HANDLE_TARGET_OPCODE(G_INSERT_VECTOR_ELT)
diff --git a/llvm/include/llvm/Target/GenericOpcodes.td b/llvm/include/llvm/Target/GenericOpcodes.td
index d967885aa2d758..8dc84fb0ba0524 100644
--- a/llvm/include/llvm/Target/GenericOpcodes.td
+++ b/llvm/include/llvm/Target/GenericOpcodes.td
@@ -1426,6 +1426,20 @@ def G_WRITE_REGISTER : GenericInstruction {
// Vector ops
//------------------------------------------------------------------------------
+// Generic insert subvector.
+def G_INSERT_SUBVECTOR : GenericInstruction {
+ let OutOperandList = (outs type0:$dst);
+ let InOperandList = (ins type0:$src0, type1:$src1, untyped_imm_0:$idx);
+ let hasSideEffects = false;
+}
+
+// Generic extract subvector.
+def G_EXTRACT_SUBVECTOR : GenericInstruction {
+ let OutOperandList = (outs type0:$dst);
+ let InOperandList = (ins type0:$src, untyped_imm_0:$idx);
+ let hasSideEffects = false;
+}
+
// Generic insertelement.
def G_INSERT_VECTOR_ELT : GenericInstruction {
let OutOperandList = (outs type0:$dst);
diff --git a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
index 28e5bf85ca9ce6..9b12d443c96e98 100644
--- a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
@@ -877,6 +877,21 @@ MachineIRBuilder::buildSelect(const DstOp &Res, const SrcOp &Tst,
return buildInstr(TargetOpcode::G_SELECT, {Res}, {Tst, Op0, Op1}, Flags);
}
+MachineInstrBuilder MachineIRBuilder::buildInsertSubvector(const DstOp &Res,
+ const SrcOp &Src0,
+ const SrcOp &Src1,
+ unsigned Idx) {
+ return buildInstr(TargetOpcode::G_INSERT_SUBVECTOR, Res,
+ {Src0, Src1, uint64_t(Idx)});
+}
+
+MachineInstrBuilder MachineIRBuilder::buildExtractSubvector(const DstOp &Res,
+ const SrcOp &Src,
+ unsigned Idx) {
+ return buildInstr(TargetOpcode::G_INSERT_SUBVECTOR, Res,
+ {Src, uint64_t(Idx)});
+}
+
MachineInstrBuilder
MachineIRBuilder::buildInsertVectorElement(const DstOp &Res, const SrcOp &Val,
const SrcOp &Elt, const SrcOp &Idx) {
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index ecb3bd33bdfd49..7041dc868cb917 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -1603,6 +1603,104 @@ void MachineVerifier::verifyPreISelGenericInstruction(const MachineInstr *MI) {
report("G_BSWAP size must be a multiple of 16 bits", MI);
break;
}
+ case TargetOpcode::G_INSERT_SUBVECTOR: {
+ const MachineOperand &Src0Op = MI->getOperand(1);
+ if (!Src0Op.isReg()) {
+ report("G_INSERT_SUBVECTOR first source must be a register", MI);
+ break;
+ }
+
+ const MachineOperand &Src1Op = MI->getOperand(2);
+ if (!Src1Op.isReg()) {
+ report("G_INSERT_SUBVECTOR second source must be a register", MI);
+ break;
+ }
+
+ const MachineOperand &IndexOp = MI->getOperand(3);
+ if (!IndexOp.isImm()) {
+ report("G_INSERT_SUBVECTOR index must be an immediate", MI);
+ break;
+ }
+
+ LLT DstTy = MRI->getType(MI->getOperand(0).getReg());
+ LLT Src0Ty = MRI->getType(Src0Op.getReg());
+ LLT Src1Ty = MRI->getType(Src1Op.getReg());
+
+ if (!DstTy.isVector()) {
+ report("Destination type must be a vector", MI);
+ break;
+ }
+
+ if (!Src0Ty.isVector()) {
+ report("First source must be a vector", MI);
+ break;
+ }
+
+ if (!Src1Ty.isVector()) {
+ report("Second source must be a vector", MI);
+ break;
+ }
+
+ if (DstTy != Src0Ty) {
+ report("Destination type must match the first source vector type", MI);
+ break;
+ }
+
+ if (Src0Ty.getElementType() != Src1Ty.getElementType()) {
+ report("Element type of source vectors must be the same", MI);
+ break;
+ }
+
+ if (IndexOp.getImm() != 0 &&
+ Src1Ty.getElementCount().getKnownMinValue() % IndexOp.getImm() != 0) {
+ report("Index must be a multiple of the second source vector's "
+ "minimum vector length",
+ MI);
+ break;
+ }
+ break;
+ }
+ case TargetOpcode::G_EXTRACT_SUBVECTOR: {
+ const MachineOperand &SrcOp = MI->getOperand(1);
+ if (!SrcOp.isReg()) {
+ report("G_EXTRACT_SUBVECTOR first source must be a register", MI);
+ break;
+ }
+
+ const MachineOperand &IndexOp = MI->getOperand(2);
+ if (!IndexOp.isImm()) {
+ report("G_EXTRACT_SUBVECTOR index must be an immediate", MI);
+ break;
+ }
+
+ LLT DstTy = MRI->getType(MI->getOperand(0).getReg());
+ LLT SrcTy = MRI->getType(SrcOp.getReg());
+
+ if (!DstTy.isVector()) {
+ report("Destination type must be a vector", MI);
+ break;
+ }
+
+ if (!SrcTy.isVector()) {
+ report("First source must be a vector", MI);
+ break;
+ }
+
+ if (DstTy.getElementType() != SrcTy.getElementType()) {
+ report("Element type of vectors must be the same", MI);
+ break;
+ }
+
+ if (IndexOp.getImm() != 0 &&
+ SrcTy.getElementCount().getKnownMinValue() % IndexOp.getImm() != 0) {
+ report("Index must be a multiple of the source vector's minimum vector "
+ "length",
+ MI);
+ break;
+ }
+
+ break;
+ }
case TargetOpcode::G_SHUFFLE_VECTOR: {
const MachineOperand &MaskOp = MI->getOperand(3);
if (!MaskOp.isShuffleMask()) {
diff --git a/llvm/test/MachineVerifier/test_g_extract_subvector.mir b/llvm/test/MachineVerifier/test_g_extract_subvector.mir
new file mode 100644
index 00000000000000..9ea54dfb981554
--- /dev/null
+++ b/llvm/test/MachineVerifier/test_g_extract_subvector.mir
@@ -0,0 +1,33 @@
+# RUN: not --crash llc -o - -mtriple=arm64 -run-pass=none -verify-machineinstrs %s 2>&1 | FileCheck %s
+# REQUIRES: aarch64-registered-target
+
+---
+name: g_extract_subvector
+tracksRegLiveness: true
+liveins:
+body: |
+ bb.0:
+ %0:_(s32) = G_CONSTANT i32 0
+ %1:_(<vscale x 2 x s32>) = G_IMPLICIT_DEF
+ %2:_(<vscale x 1 x s32>) = G_IMPLICIT_DEF
+
+ ; CHECK: G_EXTRACT_SUBVECTOR first source must be a register
+ %3:_(<vscale x 2 x s32>) = G_EXTRACT_SUBVECTOR 1, 0
+
+ ; CHECK: G_EXTRACT_SUBVECTOR index must be an immediate
+ %4:_(<vscale x 1 x s32>) = G_EXTRACT_SUBVECTOR %2, %0
+
+ ; CHECK: Destination type must be a vector
+ %5:_(s32) = G_EXTRACT_SUBVECTOR %2, 0
+
+ ; CHECK: First source must be a vector
+ %6:_(<vscale x 2 x s32>) = G_EXTRACT_SUBVECTOR %0, 0
+
+ %7:_(<vscale x 1 x s16>) = G_IMPLICIT_DEF
+
+ ; CHECK: Element type of vectors must be the same
+ %8:_(<vscale x 2 x s32>) = G_EXTRACT_SUBVECTOR %7, 0
+
+ ; CHECK: Index must be a multiple of the source vector's minimum vector length
+ %9:_(<vscale x 4 x s32>) = G_EXTRACT_SUBVECTOR %1, 3
+...
diff --git a/llvm/test/MachineVerifier/test_g_insert_subvector.mir b/llvm/test/MachineVerifier/test_g_insert_subvector.mir
new file mode 100644
index 00000000000000..5f26313b985319
--- /dev/null
+++ b/llvm/test/MachineVerifier/test_g_insert_subvector.mir
@@ -0,0 +1,44 @@
+# RUN: not --crash llc -o - -mtriple=arm64 -run-pass=none -verify-machineinstrs %s 2>&1 | FileCheck %s
+# REQUIRES: aarch64-registered-target
+
+---
+name: g_splat_vector
+tracksRegLiveness: true
+liveins:
+body: |
+ bb.0:
+ %0:_(s32) = G_CONSTANT i32 0
+ %1:_(<vscale x 2 x s32>) = G_IMPLICIT_DEF
+ %2:_(<vscale x 1 x s32>) = G_IMPLICIT_DEF
+
+ ; CHECK: G_INSERT_SUBVECTOR first source must be a register
+ %3:_(<vscale x 2 x s32>) = G_INSERT_SUBVECTOR 1, %2, 0
+
+ ; CHECK: G_INSERT_SUBVECTOR second source must be a register
+ %4:_(<vscale x 2 x s32>) = G_INSERT_SUBVECTOR %1, 1, 0
+
+ ; CHECK: G_INSERT_SUBVECTOR index must be an immediate
+ %5:_(<vscale x 2 x s32>) = G_INSERT_SUBVECTOR %1, %2, %0
+
+ ; CHECK: Destination type must be a vector
+ %6:_(s32) = G_INSERT_SUBVECTOR %1, %2, 0
+
+ ; CHECK: First source must be a vector
+ %7:_(<vscale x 2 x s32>) = G_INSERT_SUBVECTOR %0, %2, 0
+
+ ; CHECK: Second source must be a vector
+ %8:_(<vscale x 2 x s32>) = G_INSERT_SUBVECTOR %1, %0, 0
+
+ ; CHECK: Destination type must match the first source vector type
+ %9:_(<vscale x 2 x s32>) = G_INSERT_SUBVECTOR %2, %1, 0
+
+ %10:_(<vscale x 1 x s16>) = G_IMPLICIT_DEF
+
+ ; CHECK: Element type of source vectors must be the same
+ %11:_(<vscale x 2 x s32>) = G_INSERT_SUBVECTOR %1, %10, 0
+
+ %12:_(<vscale x 4 x s32>) = G_IMPLICIT_DEF
+
+ ; CHECK: Index must be a multiple of the second source vector's minimum vector length
+ %13:_(<vscale x 4 x s32>) = G_INSERT_SUBVECTOR %12, %1, 3
+...
|
G_INSERT and G_EXTRACT are not sufficient to use to represent both INSERT/EXTRACT on a subregister and INSERT/EXTRACT on a vector. We would like to be able to INSERT/EXTRACT on vectors in cases that INSERT/EXTRACT on vector subregisters are not sufficient, so we add these opcodes.
michaelmaitland
force-pushed
the
gisel-subvectors
branch
from
db00814 to
46f38f9
Compare
|
I just read the description of G_INSERT and G_EXTRACT. They sound exactly like G_INSERT_SUBVECTOR and G_EXTRACT_SUBVECTOR. They don't use a subreg index. |
Nevermind, I see now G_INSERT/EXTRACT specifically says "bit-index" |
We should also get rid of G_INSERT/G_EXTRACT |
arsenm
reviewed
Mar 9, 2024
| @@ -0,0 +1,33 @@ | |||
| # RUN: not --crash llc -o - -mtriple=arm64 -run-pass=none -verify-machineinstrs %s 2>&1 | FileCheck %s | |||
| # REQUIRES: aarch64-registered-target | |||
There was a problem hiding this comment.
Don't really need to specify aarch64, you're not referencing anything target specific
There was a problem hiding this comment.
All tests in this directory have this require. Let’s remove them together in a single follow up patch?
There was a problem hiding this comment.
I will remove this one here though!
There was a problem hiding this comment.
All tests in this directory have this require. Let’s remove them together in a single follow up patch?
I will remove this one here though!
These new tests fail cleanly without expectedly crashing on builds configured for selected targets (e.g. -DLLVM_TARGETS_TO_BUILD=ARM) without a LLVM_DEFAULT_TARGET_TRIPLE.
bin/llc: error: unable to get target for '', see --version and --triple.
I think that is why other tests in this directory have the REQUIRES along with a valid triple; either in command line (-mtriple=aarch64) or in the test file itself (target triple = "aarch64--").
I think we need G_EXTRACT as the equivalent of EXTRACT_SUBREG in addition to G_EXTRACT_SUBVECTOR |
Yes, it is the equivalent of EXTRACT_SUBREG, which is why it's problematic. It's essentially unlegalizable since it's too unrestricted |
arsenm
approved these changes
Mar 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
G_INSERT and G_EXTRACT are not sufficient to use to represent both INSERT/EXTRACT on a subregister and INSERT/EXTRACT on a vector.
We would like to be able to INSERT/EXTRACT on vectors in cases that INSERT/EXTRACT on vector subregisters are not sufficient, so we add these opcodes.
I tried to do a patch where we treated G_EXTRACT as both G_EXTRACT_SUBVECTOR and G_EXTRACT_SUBREG, but ran into an infinite loop at this point in the SDAG equivalent code.