[Generic][AIE2] Combiner for shufflevectors that use build vector #129
Conversation
ValentijnvdBeek
added
llvm:globalisel
ValentijnvdBeek
requested review from
abhinay-anubola,
abnikant,
andcarminati,
gbossu,
khallouh,
konstantinschwarz,
martien-de-jong,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| return true; | ||
| default: | ||
| return false; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| /// Represents a G_SHUFFLE_VECTOR | ||
| class GShuffleVector : public GMergeLikeInstr { |
There was a problem hiding this comment.
G_SHUFFLE_VECTOR takes the shufflemask as the last operand, which shouldn't be part of the getNumSources I think?
To me that's what makes G_SHUFFLE_VECTOR different compared to the other "merge like" instructions.
I'd rather not make it part of that group, as this can have surprising effects to other existing combines
There was a problem hiding this comment.
I can see that argument. I was looking at it from the perspective of what the result of the function would be. I'll separate them, thanks.
| // Our inputs need to be either be build vectors or undefined, register inputs | ||
| // break this optimization. You could maybe do something clever were you | ||
| // concatenate vectors to save half a build vector. | ||
| if ((SrcInstr1 == 0 && IsUndef1 == 0) || (SrcInstr2 == 0 && IsUndef2 == 0)) |
There was a problem hiding this comment.
These are pointers, just use (!SrcInstr1 && !IsUndef1) || ...)
There was a problem hiding this comment.
Sure, will do. I personally prefer explicit NULL pointer checking for clarity (which, to be fair, this isn't yet), but doing it like that is fine with me.
| ; CHECK: [[DEF:%[0-9]+]]:_(<16 x s32>) = G_IMPLICIT_DEF | ||
| ; CHECK-NEXT: $x0 = COPY [[DEF]](<16 x s32>) | ||
| ; CHECK-NEXT: PseudoRET implicit $lr, implicit $x0 | ||
| %0:_(s32) = G_CONSTANT i32 28 |
There was a problem hiding this comment.
Nit: unused G_CONSTANTs here and in the tests below
There was a problem hiding this comment.
Fair, thanks Konstantin.
| %5:_(s32) = G_IMPLICIT_DEF | ||
| %2:_(<8 x s32>) = G_BUILD_VECTOR %1(s32), %5(s32), %5(s32), %5(s32), %5(s32), %5(s32), %5(s32), %5(s32) | ||
| %0:_(<8 x s32>) = G_SHUFFLE_VECTOR %2(<8 x s32>), %3, shufflemask(0, 0, 0, 0, 0, 0, 0, 0) | ||
| PseudoRET implicit $lr, implicit %0 |
There was a problem hiding this comment.
Looks like this is missing the closing ... at the end, and then I think this would fail, because this is further combined into G_AIE_BROADCAST?
There was a problem hiding this comment.
The shufflevector branch has become really divergent in the past month or so, so that hasn't landed yet. Let's try to get all of that merged as quickly as we can, I'll manually merge them to make sure that spectests are updated before enter the main branch.
Missing the closing ... is deliberate, trailing ellipses have caused errors with the unit tests scripts for me before.
There was a problem hiding this comment.
Ran into it again just now. If you have a trailing ellipsis, you need a trailing newline, or otherwise the update mir script will error out. If you skip it, then it doesn't matter whether you have a newline or not. That is why I tend to commit mine without the trailing ellipsis.
llvm/test/CodeGen/AIE/aie2/GlobalISel/prelegalizercombiner-shufflevector-buildvector.mir
Outdated
Show resolved
Hide resolved
ValentijnvdBeek
force-pushed
the
vvandebe.sv.bv.combiner
branch
from
e0a5aae to
cc0f633
Compare
This implements the simple legalization that lowers G_SHUFFLE_VECTOR into extracts of the elements based on the mask and then combining them using a G_BUILD_VECTOR. Our architecture has a VSHUFFLE instruction which could be used to implement some patterns more efficiently.
… CONCAT_VECTOR We check for iterative shift masks which corresponds to the CONCAT_VECTOR instruction.
…hunks of a vector
… of two vectors together
ValentijnvdBeek
force-pushed
the
vvandebe.vshuffle.impl
branch
from
5654047 to
84f3995
Compare
Transforms a shufflevector that uses a build vector or undefined
into just a build vector. This can be done is because a shuffle
vector lowering is an unmerge and then merge. Since build is a
merge, the merge and unmerge cancel each other out and we can just
merge the vector directly.
Example:
```
%1:_(s32) = COPY $r0
%3:_(<8 x s32>) = G_IMPLICIT_DEF
%5:_(s32) = G_IMPLICIT_DEF
%2:_(<8 x s32>) = G_BUILD_VECTOR %1(s32), %5(s32), %5(s32), %5(s32), %5(s32), %5(s32), %5(s32), %5(s32)
%0:_(<8 x s32>) = G_SHUFFLE_VECTOR %2(<8 x s32>), %3, shufflemask(0, 0, 0, 0, 0, 0, 0, 0)
===>
%2:_(<8 x s32>) = G_BUILD_VECTOR %1(s32), %1(s32), %1(s32), %1(s32), %1(s32), %1(s32), %1(s32), %1(s32)
```
ValentijnvdBeek
force-pushed
the
vvandebe.sv.bv.combiner
branch
from
cc0f633 to
fee2d99
Compare
| select_to_minmax, or_to_bsp, combine_concat_vector, | ||
| commute_constant_to_rhs]> { | ||
| commute_constant_to_rhs, shufflevector_merge]> { | ||
| } |
There was a problem hiding this comment.
This is needed since some ARM64 tests relies on the legalizer changing the inputs of the shufflevector and if you don't run them afterwards you get worse code.
ValentijnvdBeek
force-pushed
the
vvandebe.vshuffle.impl
branch
from
84f3995 to
5c3b1a6
Compare
| ; CHECK-NEXT: mov.h v0[1], v1[0] | ||
| ; CHECK-NEXT: mov w8, #0 ; =0x0 | ||
| ; CHECK-NEXT: fmov s0, w8 | ||
| ; CHECK-NEXT: mov.16b v1, v0 |
There was a problem hiding this comment.
Add copyright header!
ValentijnvdBeek
force-pushed
the
vvandebe.vshuffle.impl
branch
3 times, most recently
from
aec1600 to
d1d0a3a
Compare
| def shufflevector_merge_matchinfo : GIDefMatchData<"SmallVector<Register, 8>">; | ||
| def shufflevector_merge : GICombineRule< | ||
| (defs root:$d, shufflevector_merge_matchinfo:$info), | ||
| (match (wip_match_opcode G_SHUFFLE_VECTOR): $d, |
There was a problem hiding this comment.
A recent move in LLVM is that you shouldn't use wip_match_opcode anymore since it slows down compilation.
Transforms a shufflevector that uses a build vector or undefined into just a build vector. This can be done is because a shuffle vector lowering is an unmerge and then merge. Since build is a merge, the merge and unmerge cancel each other out and we can just merge the vector directly.
Example: