[PATCH] D39823: GlobalISel: Enable the legalization of G_MERGE_VALUES and G_UNMERGE_VALUES

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: include/llvm/Target/GenericOpcodes.td:514
 
 /// Concatenante multiple registers of the same size into a wider register.
 def G_MERGE_VALUES : Instruction {
----------------
I know this isn't from your patch but Concatenante -> Concatenate


================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:185-189
+  if (MI.getOpcode() == TargetOpcode::G_UNMERGE_VALUES && TypeIdx == 1)
+    Reg = MI.getOperand(MI.getNumOperands() - 1).getReg();
+  else
+    Reg = MI.getOperand(OpIdx).getReg();
+  return MRI.getType(Reg);
----------------
This is a nitpick but we should early-return here


================
Comment at: test/CodeGen/AArch64/GlobalISel/legalize-merge-values.mir:1
+# RUN: llc -O0 -run-pass=legalizer -global-isel -global-isel-abort=0 -pass-remarks-missed='gisel*' %s -o - 2>&1 | FileCheck %s
+
----------------
Could you add a comment explaining the intent of this test?


================
Comment at: test/CodeGen/AArch64/GlobalISel/legalize-nonpowerof2eltsvec.mir:23-33
     ; CHECK-LABEL: name: test_legalize_merge_v3s32
-    ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY %w0
-    ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY %w1
-    ; CHECK: [[COPY2:%[0-9]+]]:_(s32) = COPY %w2
-    ; CHECK: %w0 = COPY [[COPY]](s32)
-    ; CHECK: %w1 = COPY [[COPY1]](s32)
-    ; CHECK: %w2 = COPY [[COPY2]](s32)
-    %0(s32) = COPY %w0
-    %1(s32) = COPY %w1
-    %2(s32) = COPY %w2
-    %3(<3 x s32>) = G_MERGE_VALUES %0(s32), %1(s32), %2(s32)
-    %4:_(s32), %5:_(s32), %6:_(s32) = G_UNMERGE_VALUES %3
-    %w0 = COPY %4
-    %w1 = COPY %5
-    %w2 = COPY %6
+    ; CHECK: [[COPY:%[0-9]+]]:_(s64) = COPY %x0
+    ; CHECK: [[MV:%[0-9]+]]:_(<3 x s64>) = G_MERGE_VALUES [[COPY]](s64), [[COPY]](s64), [[COPY]](s64)
+    ; CHECK: [[COPY1:%[0-9]+]]:_(<3 x s64>) = COPY [[MV]](<3 x s64>)
+    ; CHECK: [[UV:%[0-9]+]]:_(s64), [[UV1:%[0-9]+]]:_(s64), [[UV2:%[0-9]+]]:_(s64) = G_UNMERGE_VALUES [[COPY1]](<3 x s64>)
+    ; CHECK: %x0 = COPY [[UV]](s64)
+    %0(s64) = COPY %x0
----------------
I'm not sure why this test has changed from s32's to s64's. Could you explain?

Also, the name is inconsistent with the types used


https://reviews.llvm.org/D39823