-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[GlobalIsel] Cleanup G_EXTRACT_VECTOR_ELT combines #109047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-llvm-globalisel Author: Thorsten Schütt (tschuett) ChangesFull diff: https://github.com/llvm/llvm-project/pull/109047.diff 3 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h b/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
index 3261b26e74cd71..1729d606f6cc5a 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
@@ -845,7 +845,8 @@ class CombinerHelper {
bool matchExtractVectorElement(MachineInstr &MI, BuildFnTy &MatchInfo);
/// Combine extract vector element with a build vector on the vector register.
- bool matchExtractVectorElementWithBuildVector(const MachineOperand &MO,
+ bool matchExtractVectorElementWithBuildVector(const MachineInstr &MI,
+ const MachineInstr &MI2,
BuildFnTy &MatchInfo);
/// Combine extract vector element with a build vector trunc on the vector
@@ -855,7 +856,8 @@ class CombinerHelper {
/// Combine extract vector element with a shuffle vector on the vector
/// register.
- bool matchExtractVectorElementWithShuffleVector(const MachineOperand &MO,
+ bool matchExtractVectorElementWithShuffleVector(const MachineInstr &MI,
+ const MachineInstr &MI2,
BuildFnTy &MatchInfo);
/// Combine extract vector element with a insert vector element on the vector
diff --git a/llvm/include/llvm/Target/GlobalISel/Combine.td b/llvm/include/llvm/Target/GlobalISel/Combine.td
index e75cf0b7d4afc1..be33c77adffb51 100644
--- a/llvm/include/llvm/Target/GlobalISel/Combine.td
+++ b/llvm/include/llvm/Target/GlobalISel/Combine.td
@@ -1373,110 +1373,23 @@ def extract_vector_element_different_indices : GICombineRule<
[{ return Helper.matchExtractVectorElementWithDifferentIndices(${root}, ${matchinfo}); }]),
(apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-def extract_vector_element_build_vector2 : GICombineRule<
+def extract_vector_element_build_vector : GICombineRule<
(defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector3 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector4 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector5 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector6 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b, $c),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector7 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b, $c, $d),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector8 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b, $c, $d, $e),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector9 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b, $c, $d, $e, $f),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector10 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b, $c, $d, $e, $f, $g),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector11 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b, $c, $d, $e, $f, $g, $h),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector12 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b, $c, $d, $e, $f, $g, $h, $i),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector13 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b, $c, $d, $e, $f, $g, $h, $i, $j),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
+ (match (G_CONSTANT $idx, $imm),
+ (G_BUILD_VECTOR $src, GIVariadic<>:$unused):$Build,
+ (G_EXTRACT_VECTOR_ELT $root, $src, $idx):$Extract,
+ [{ return Helper.matchExtractVectorElementWithBuildVector(*${Extract}, *${Build},
+ ${matchinfo}); }]),
+ (apply [{ Helper.applyBuildFn(*${Extract}, ${matchinfo}); }])>;
-def extract_vector_element_build_vector14 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b, $c, $d, $e, $f, $g, $h, $i, $j, $k),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector15 : GICombineRule<
+def extract_vector_element_shuffle_vector : GICombineRule<
(defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b, $c, $d, $e, $f, $g, $h, $i, $j, $k, $l),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
-def extract_vector_element_build_vector16 : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_BUILD_VECTOR $src, $x, $y, $z, $a, $b, $c, $d, $e, $f, $g, $h, $i, $j, $k, $l, $m),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
+ (match (G_CONSTANT $idx, $imm),
+ (G_SHUFFLE_VECTOR $src, $src1, $src2, $mask):$Shuffle,
+ (G_EXTRACT_VECTOR_ELT $root, $src, $idx):$Extract,
+ [{ return Helper.matchExtractVectorElementWithShuffleVector(*${Extract}, *${Shuffle},
+ ${matchinfo}); }]),
+ (apply [{ Helper.applyBuildFn(*${Extract}, ${matchinfo}); }])>;
def extract_vector_element_build_vector_trunc2 : GICombineRule<
(defs root:$root, build_fn_matchinfo:$matchinfo),
@@ -1547,13 +1460,6 @@ def nneg_zext : GICombineRule<
[{ return Helper.matchNonNegZext(${root}, ${matchinfo}); }]),
(apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-def extract_vector_element_shuffle_vector : GICombineRule<
- (defs root:$root, build_fn_matchinfo:$matchinfo),
- (match (G_SHUFFLE_VECTOR $src, $src1, $src2, $mask),
- (G_EXTRACT_VECTOR_ELT $root, $src, $idx),
- [{ return Helper.matchExtractVectorElementWithShuffleVector(${root}, ${matchinfo}); }]),
- (apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>;
-
// Combines concat operations
def concat_matchinfo : GIDefMatchData<"SmallVector<Register>">;
def combine_concat_vector : GICombineRule<
@@ -1647,20 +1553,7 @@ match_extract_of_element,
insert_vector_elt_oob,
extract_vector_element_not_const,
extract_vector_element_different_indices,
-extract_vector_element_build_vector2,
-extract_vector_element_build_vector3,
-extract_vector_element_build_vector4,
-extract_vector_element_build_vector5,
-extract_vector_element_build_vector7,
-extract_vector_element_build_vector8,
-extract_vector_element_build_vector9,
-extract_vector_element_build_vector10,
-extract_vector_element_build_vector11,
-extract_vector_element_build_vector12,
-extract_vector_element_build_vector13,
-extract_vector_element_build_vector14,
-extract_vector_element_build_vector15,
-extract_vector_element_build_vector16,
+extract_vector_element_build_vector,
extract_vector_element_build_vector_trunc2,
extract_vector_element_build_vector_trunc3,
extract_vector_element_build_vector_trunc4,
diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelperVectorOps.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelperVectorOps.cpp
index 66b1c5f8ca82c7..84fb3b59658956 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelperVectorOps.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelperVectorOps.cpp
@@ -146,9 +146,9 @@ bool CombinerHelper::matchExtractVectorElementWithDifferentIndices(
}
bool CombinerHelper::matchExtractVectorElementWithBuildVector(
- const MachineOperand &MO, BuildFnTy &MatchInfo) {
- MachineInstr *Root = getDefIgnoringCopies(MO.getReg(), MRI);
- GExtractVectorElement *Extract = cast<GExtractVectorElement>(Root);
+ const MachineInstr &MI, const MachineInstr &MI2, BuildFnTy &MatchInfo) {
+ const GExtractVectorElement *Extract = cast<GExtractVectorElement>(&MI);
+ const GBuildVector *Build = cast<GBuildVector>(&MI2);
//
// %zero:_(s64) = G_CONSTANT i64 0
@@ -160,23 +160,8 @@ bool CombinerHelper::matchExtractVectorElementWithBuildVector(
// %extract:_(32) = COPY %arg1(s32)
//
//
- //
- // %bv:_(<2 x s32>) = G_BUILD_VECTOR %arg1(s32), %arg2(s32)
- // %extract:_(s32) = G_EXTRACT_VECTOR_ELT %bv(<2 x s32>), %opaque(s64)
- //
- // -->
- //
- // %bv:_(<2 x s32>) = G_BUILD_VECTOR %arg1(s32), %arg2(s32)
- // %extract:_(s32) = G_EXTRACT_VECTOR_ELT %bv(<2 x s32>), %opaque(s64)
- //
Register Vector = Extract->getVectorReg();
-
- // We expect a buildVector on the Vector register.
- GBuildVector *Build = getOpcodeDef<GBuildVector>(Vector, MRI);
- if (!Build)
- return false;
-
LLT VectorTy = MRI.getType(Vector);
// There is a one-use check. There are more combines on build vectors.
@@ -185,14 +170,7 @@ bool CombinerHelper::matchExtractVectorElementWithBuildVector(
!getTargetLowering().aggressivelyPreferBuildVectorSources(Ty))
return false;
- Register Index = Extract->getIndexReg();
-
- // If the Index is constant, then we can extract the element from the given
- // offset.
- std::optional<ValueAndVReg> MaybeIndex =
- getIConstantVRegValWithLookThrough(Index, MRI);
- if (!MaybeIndex)
- return false;
+ APInt Index = getIConstantFromReg(Extract->getIndexReg(), MRI);
// We now know that there is a buildVector def'd on the Vector register and
// the index is const. The combine will succeed.
@@ -200,7 +178,7 @@ bool CombinerHelper::matchExtractVectorElementWithBuildVector(
Register Dst = Extract->getReg(0);
MatchInfo = [=](MachineIRBuilder &B) {
- B.buildCopy(Dst, Build->getSourceReg(MaybeIndex->Value.getZExtValue()));
+ B.buildCopy(Dst, Build->getSourceReg(Index.getZExtValue()));
};
return true;
@@ -274,9 +252,9 @@ bool CombinerHelper::matchExtractVectorElementWithBuildVectorTrunc(
}
bool CombinerHelper::matchExtractVectorElementWithShuffleVector(
- const MachineOperand &MO, BuildFnTy &MatchInfo) {
- GExtractVectorElement *Extract =
- cast<GExtractVectorElement>(getDefIgnoringCopies(MO.getReg(), MRI));
+ const MachineInstr &MI, const MachineInstr &MI2, BuildFnTy &MatchInfo) {
+ const GExtractVectorElement *Extract = cast<GExtractVectorElement>(&MI);
+ const GShuffleVector *Shuffle = cast<GShuffleVector>(&MI2);
//
// %zero:_(s64) = G_CONSTANT i64 0
@@ -302,32 +280,12 @@ bool CombinerHelper::matchExtractVectorElementWithShuffleVector(
// %extract:_(s32) = G_IMPLICIT_DEF
//
//
- //
- //
- //
- // %sv:_(<4 x s32>) = G_SHUFFLE_SHUFFLE %arg1(<4 x s32>), %arg2(<4 x s32>),
- // shufflemask(0, 0, 0, -1)
- // %extract:_(s32) = G_EXTRACT_VECTOR_ELT %sv(<4 x s32>), %opaque(s64)
- //
- // -->
- //
- // %sv:_(<4 x s32>) = G_SHUFFLE_SHUFFLE %arg1(<4 x s32>), %arg2(<4 x s32>),
- // shufflemask(0, 0, 0, -1)
- // %extract:_(s32) = G_EXTRACT_VECTOR_ELT %sv(<4 x s32>), %opaque(s64)
- //
-
- // We try to get the value of the Index register.
- std::optional<ValueAndVReg> MaybeIndex =
- getIConstantVRegValWithLookThrough(Extract->getIndexReg(), MRI);
- if (!MaybeIndex)
- return false;
- GShuffleVector *Shuffle =
- cast<GShuffleVector>(getDefIgnoringCopies(Extract->getVectorReg(), MRI));
+ APInt Index = getIConstantFromReg(Extract->getIndexReg(), MRI);
ArrayRef<int> Mask = Shuffle->getMask();
- unsigned Offset = MaybeIndex->Value.getZExtValue();
+ unsigned Offset = Index.getZExtValue();
int SrcIdx = Mask[Offset];
LLT Src1Type = MRI.getType(Shuffle->getSrc1Reg());
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit message should be more descriptive
// | ||
// | ||
// | ||
// %sv:_(<4 x s32>) = G_SHUFFLE_SHUFFLE %arg1(<4 x s32>), %arg2(<4 x s32>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this comment was lost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case is not valid anymore.
@@ -160,23 +160,8 @@ bool CombinerHelper::matchExtractVectorElementWithBuildVector( | |||
// %extract:_(32) = COPY %arg1(s32) | |||
// | |||
// | |||
// | |||
// %bv:_(<2 x s32>) = G_BUILD_VECTOR %arg1(s32), %arg2(s32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lost comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The index is never opaque
. It is always a constant due to the pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case is not valid anymore.
The title should be more descriptive. "Hygiene" is not useful for the future us. Please fix it to include more context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning this up, it's always irked me a little that these didn't scale to larger vector sizes. I think that could make this non-nfc for some larger vectors?
(G_EXTRACT_VECTOR_ELT $root, $src, $idx), | ||
[{ return Helper.matchExtractVectorElementWithBuildVector(${root}, ${matchinfo}); }]), | ||
(apply [{ Helper.applyBuildFnMO(${root}, ${matchinfo}); }])>; | ||
(match (G_CONSTANT $idx, $imm), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it pass imm to avoid the need to call getIConstantFromReg(Extract->getIndexReg(), MRI);
, or does that not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APInt llvm::getIConstantFromReg(Register Reg, const MachineRegisterInfo &MRI) {
MachineInstr *Const = MRI.getVRegDef(Reg);
assert((Const && Const->getOpcode() == TargetOpcode::G_CONSTANT) &&
"expected a G_CONSTANT on Reg");
return Const->getOperand(1).getCImm()->getValue();
}
Sure everything is possible, but we would have to play with the low-level Cimm details. I would like to avoid that.
Ok, you got me. It is indeed non-nfc changes. It just did not show up in the tests. Bad test coverage, blame me.
|
%idx:_(s64) = G_CONSTANT i64 0 | ||
%arg1:_(s64) = COPY $x0 | ||
%arg2:_(s64) = COPY $x1 | ||
%bv:_(<18 x s64>) = G_BUILD_VECTOR %arg1(s64), %arg2(s64), %arg1(s64), %arg2(s64), %arg1(s64), %arg2(s64), %arg1(s64), %arg2(s64), %arg1(s64), %arg2(s64), %arg1(s64), %arg2(s64), %arg1(s64), %arg2(s64), %arg1(s64), %arg2(s64), %arg1(s64), %arg2(s64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing patterns covered up to 16. This should have been a negative test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if there are no other comments.
Reduce duplicated build vector patterns by exploiting variadic args. Make index parameter const to improve hit rate. Use `getIConstantFromReg` to retrieve immediate because they are not fallible anymore. Improve extraction from build vector and shuffle vector.
Reduce duplicated build vector patterns by exploiting variadic args.
Make index parameter const to improve hit rate.
Use
getIConstantFromReg
to retrieve immediate because they are not fallible anymore.Improve extraction from build vector and shuffle vector.