[PATCH] D104355: [GlobalISel] Add a new artifact combiner for unmerge which looks through general artifact expressions.

[Amara Emerson via Phabricator via llvm-commits]

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