[PATCH] D107650: GlobalISel[RFC]: Avoid use of G_INSERT and G_EXTRACT in Legalizer

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic added a comment.

> Haven't finished grokking this yet, some comments so far.

Do you have some test cases you are interested in?  High level summary would be that by doing unmerge to each element before changing number of elements in a vector we are able to reference each element using vreg. This should be enough since we don't deal with anything smaller. This fixes main problem I see in combines "referencing an element in subvector" required by "merge subvector with subvector(or single element) of another subvector".



================
Comment at: llvm/lib/CodeGen/GlobalISel/Legalizer.cpp:229-234
+    ++IterCount;
+    if (IterCount == 1000) {
+      llvm_unreachable(("infinite loop in " + MF.getName().str() +
+                        " , TODO: fix mir tests\n\n")
+                           .c_str());
+    }
----------------
aemerson wrote:
> ?
This stops infinite loops that happen on many mir tests that I did not update yet. I would like to get some feedback first and update tests according to strategy we are going to implement (see irtranslator-function-args.ll for example I need to change all g_concats like that into unmerge in all mir tests).
Infinite loop is caused by legalization of artifact that cannot be combined. Basically lower into same thing that did not combine in the first place (eventually runs out of memory).


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:2876
 
+  if (MemTy.isVector() && DstTy.isVector()) {
+    LLT EltTy = MemTy.getScalarType();
----------------
aemerson wrote:
> Why is this code needed? Can this be a follow up change?
This was easiest way to get all loads/stores to work for amdgpu (complicated fewerElementsIf is used at the moment).
> Can this be a follow up change?
I don't thnik so. We need all places that deal with vectors to use new approach (including reduceLoadStoreWidth which is on todo list). I expect to be able to deal with vectors of any numberOfElements in artifact combiner after this patch. Here is a brief explanation of the code bellow : 
- load lowering will load smaller pieces of legal sizes, unmerge each piece to vector_elts and create large merge
- store lowering unmerges large store to vector_elts, them merges elements into pieces of legal store size


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107650/new/

https://reviews.llvm.org/D107650