[PATCH] D88122: [GlobalISel] Add widenScalar support for G_CONCAT_VECTORS and use for legalization v2s32 G_ICMPs

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizerHelper.h:165-166
 private:
-  LegalizeResult
-  widenScalarMergeValues(MachineInstr &MI, unsigned TypeIdx, LLT WideTy);
-  LegalizeResult
-  widenScalarUnmergeValues(MachineInstr &MI, unsigned TypeIdx, LLT WideTy);
-  LegalizeResult
-  widenScalarExtract(MachineInstr &MI, unsigned TypeIdx, LLT WideTy);
-  LegalizeResult
-  widenScalarInsert(MachineInstr &MI, unsigned TypeIdx, LLT WideTy);
-  LegalizeResult
-  widenScalarAddSubShlSat(MachineInstr &MI, unsigned TypeIdx, LLT WideTy);
+  LegalizeResult widenScalarConcatVectors(MachineInstr &MI, unsigned TypeIdx,
+                                          LLT WideTy);
+  LegalizeResult widenScalarMergeValues(MachineInstr &MI, unsigned TypeIdx,
----------------
I'm not sure why all of these lines changed?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3941-3947
+  case G_TRUNC: {
+    LLT DstTy = MRI.getType(MI.getOperand(0).getReg());
+    LLT NewNarrowTy = NarrowTy;
+    if (NarrowTy.isVector())
+      LLT::vector(NarrowTy.getNumElements(), DstTy.getElementType());
+    return reduceOperationWidth(MI, 0, NewNarrowTy);
+  }
----------------
I think this deserves to be split into a separate patch


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3945
+    if (NarrowTy.isVector())
+      LLT::vector(NarrowTy.getNumElements(), DstTy.getElementType());
+    return reduceOperationWidth(MI, 0, NewNarrowTy);
----------------
This doesn't do anything, I assume this should have a NewNarrowTy =


================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/legalize-vector-icmp.mir:1959
+    RET_ReallyLR implicit $d0
+...
----------------
Can you add a case with an <8 x s32> icmp result type


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88122