[AIE2] Simplify sequence of VInserts to G_BUILD_VECTOR #187
Conversation
abhinay-anubola
force-pushed
the
sanubola.simplify.VINSERT.sequence
branch
3 times, most recently
from
1aa3088 to
5b7e112
Compare
abhinay-anubola
force-pushed
the
sanubola.simplify.VINSERT.sequence
branch
6 times, most recently
from
6395c61 to
46de687
Compare
abhinay-anubola
requested review from
abnikant,
gbossu,
khallouh,
konstantinschwarz,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
abhinay-anubola
force-pushed
the
sanubola.simplify.VINSERT.sequence
branch
from
46de687 to
34ab0eb
Compare
llvm/test/CodeGen/AIE/aie2/GlobalISel/inst-select-aie-add-vector-elt-left.mir
Show resolved
Hide resolved
llvm/test/CodeGen/AIE/aie2/GlobalISel/legalize-build-vector-trunc.mir
Outdated
Show resolved
Hide resolved
|
I ran QoR with the updated changes, but the results remain unchanged compared to the previous run. |
abhinay-anubola
force-pushed
the
sanubola.simplify.VINSERT.sequence
branch
from
bfbb569 to
3cf078b
Compare
|
LGTM, I just think it is important to fix #187 (comment) |
abhinay-anubola
force-pushed
the
sanubola.simplify.VINSERT.sequence
branch
from
3cf078b to
2f616e5
Compare
|
I looked at QoR and this seems to cause regressions more than improvements, I'd like to understand them before merging |
abhinay-anubola
force-pushed
the
sanubola.simplify.VINSERT.sequence
branch
2 times, most recently
from
df22faa to
7b6fdc3
Compare
| Register Dst = MRI.createGenericVirtualRegister(VecTy); | ||
|
|
||
| if (DstVecSize == 512 && Operand == OperandBegin + 1) | ||
| if (DstVecSize == 512 && Operand == OperandBegin) { | ||
| Dst = DstReg; |
There was a problem hiding this comment.
nit: we've created an unused virtual register.
There was a problem hiding this comment.
Is this about the register Dst, we have used later.
| const Register ExtOpIdxReg = ExtOp->getOperand(3).getReg(); | ||
| auto SetOpCst = getIConstantVRegValWithLookThrough(SetOpIdxReg, MRI); | ||
| auto ExtOpCst = getIConstantVRegValWithLookThrough(ExtOpIdxReg, MRI); | ||
| if (SetOpIdxReg != ExtOpIdxReg && |
There was a problem hiding this comment.
nit: if they are the same, we only need one getIConstantVRegValWithLookThrough and if we don't have SetOpCst, we don't need to find ExtOpCst.
There was a problem hiding this comment.
In that case we will end up having two if blocks.
abhinay-anubola
force-pushed
the
sanubola.simplify.VINSERT.sequence
branch
from
7b6fdc3 to
a778ef1
Compare
|
|
||
| // Combining Set and Extract to fetch next VInsert | ||
| if (IsSet(CurMI) && tryToCombineSetExtract(*CurMI)) | ||
| CurMI = getDefIgnoringCopies(SrcReg, MRI); |
There was a problem hiding this comment.
Isn't tryToCombineSetExtract potentially deleting CurMI? Is it always safe to call CurMI = getDefIgnoringCopies(SrcReg, MRI);?
llvm/test/CodeGen/AIE/aie2/GlobalISel/combine-vinsert-sequence-prelegalizer.mir
Outdated
Show resolved
Hide resolved
| Regs.push_back(It->second); | ||
| } | ||
| Register DstRegTrunc = MRI.createGenericVirtualRegister( | ||
| LLT::fixed_vector(DstRegLen, SclSrcBits)); |
There was a problem hiding this comment.
Could we re-use the type from MI.getOperand(0) instead of trying to reconstruct it with LLT::fixed_vector(DstRegLen, SclSrcBits)?
There was a problem hiding this comment.
We can't do that, as they both are not same. In a case <16 x s32> is type of MI.getOperand(0) and we need <16 x s8> for buildBuildVectorTrunc
There was a problem hiding this comment.
Why is it like this actually, do our VINSERT.S8 intrinsics take a <16 x s32> type?
There was a problem hiding this comment.
yes, they take <16 x s32> type
llvm/test/CodeGen/AIE/aie2/GlobalISel/combine-vinsert-sequence-prelegalizer.mir
Outdated
Show resolved
Hide resolved
abhinay-anubola
force-pushed
the
sanubola.simplify.VINSERT.sequence
branch
from
a778ef1 to
f0efd5b
Compare
abhinay-anubola
force-pushed
the
sanubola.simplify.VINSERT.sequence
branch
from
f0efd5b to
ecd3fd7
Compare
Simplify sequence of VInserts by legalizing G_BUILD_VECTOR_TRUNC and combining VInsert sequence in PreLegalizerCombiner into G_BUILD_VECTOR_TRUNC