[PATCH] D104355: [GlobalISel] Add a new artifact combiner for unmerge which looks through general artifact expressions.
Amara Emerson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 16 08:08:33 PDT 2021
aemerson added inline comments.
================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h:645-646
+
+ // Only support finding values from either the container source or the
+ // inserted reg, can't find the value if it spans both.
+ // E.g. the following is disallowed:
----------------
Petar.Avramovic wrote:
> I don't think we want to combine G_INSERT but instead not generate them at all. Is this insert from ir or from legalizer?
> >can't find the value if it spans both.
> Did you consider this case? I had also encountered infinite loops when trying to deal with this combine when one of the vector sizes is odd.
> I wanted to avoid making G_INSERT and use LCM merge/unmerge pad with undef instead. AMDGPU can deal with merge/unmerge when sizes are even (in the end it would turn everything into vectors of size 2 in worst case).
> With odd vector size at some point it has to do "pad with undef". U guess that we could make combine that recognizes value that is spanned over real element and undef but it looks unnecessary complicated to combine.
>
> I thought about adding new artifacts equivalent to G_ANYEXT/G_TRUNC but for vectors (these would be simplified versions of sdag's INSERT_SUBVECTOR and EXTRACT_SUBVECTOR) with intent to be trivial to combine. They would pad vector with undef elements/trunc elements. These do not exist in llvm-ir, and when used in more/fewer_elements_vector would always end up combining with each other if we consistently ask for same number of elements in all more/fewer_elements_vector legalizer rules.
> Would this work for you (I would expect that there would be no need to combine insert)?
>
>
>
It’s from moreElementsVector.
Perhaps true, but I didn’t want to tackle that problem of moving away from G_INSERT in this patch. The algorithm is agnostic to the implementation of moreElements, so my plan was to support inserts first and then later remove the code if/when G_INSERT is removed.
> Did you consider this case? I had also encountered infinite loops when trying to deal with this combine when one of the vector sizes is odd.
I didn’t, because it wasn’t clear when this would actually occur. The inserts I’ve seen are mostly just being used to widen a type.
In principle I think it’s a good idea to move away from G_INSERT/EXTRACT but it doesn’t really affect the purpose of this patch, which is to deal with unmerges of unmerges.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104355/new/
https://reviews.llvm.org/D104355
More information about the llvm-commits
mailing list