[PATCH] D114198: [GlobalISel] Rework more/fewer elements for vectors

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:995
+  case TargetOpcode::G_FREEZE: {
+    if (TypeIdx != 0)
+      return UnableToLegalize;
----------------
arsenm wrote:
> Why this change? I thought this previously handled arbitrary cases, and this doesn't?
reduceOperationWidth did number of elements splits and narrow scalar for freeze, here I extracted narrow scalar only.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3647
+    std::initializer_list<unsigned> NonVecOpIndices) {
+  if (MI.getNumMemOperands() != 0)
+    return false;
----------------
arsenm wrote:
> Not sure what the point of this check is
This is for load/store with vector pointer address. the whole function is annoying check that we don't try to split operands of non-intended Opcode.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:6870
-      auto IdxK = MIRBuilder.buildConstant(IdxTy, ExtractIdx);
-      auto Extract = MIRBuilder.buildExtractVectorElement(EltTy, SrcVec, IdxK);
-      BuildVec.push_back(Extract.getReg(0));
----------------
arsenm wrote:
> I think it's more appropriate to lower the vector operation in terms of G_EXTRACT_VECTOR_ELT and not direct to the register extracts, G_SHUFFLE_VECTOR is not a legalization artifact
Sure, we do unmerge via custom legalize of G_EXTRACT_VECTOR_ELT


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4696-4712
+    if (IsTFE) {
+      if (ResultRegs.size() == 1) {
+        NewResultReg = ResultRegs[0];
+      } else if (ResultRegs.size() == 2) {
+        LLT V4S16 = LLT::fixed_vector(4, 16);
+        NewResultReg = B.buildConcatVectors(V4S16, ResultRegs).getReg(0);
+      } else {
----------------
arsenm wrote:
> This is a lot longer, is there a simpler way?
I don't see it. Whole function is complicated, handles many cases, and packs all output operands into ResultRegs. And we no longer pad with undef in all cases.


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

https://reviews.llvm.org/D114198