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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:67-88
+
+  // Set default actions for G_MERGE_VALUES and G_UNMERGE_VALUES.
+  // TODO: Remove this once we have better support for vector types.
+  for (unsigned Op :
+       {TargetOpcode::G_MERGE_VALUES, TargetOpcode::G_UNMERGE_VALUES})
+    for (unsigned TypeIdx : {0, 1}) {
+      setScalarAction(Op, TypeIdx, {{1, Legal}});
----------------
This is in the class that contains target-independent info on how to legalize operations and types.
Therefore, I don't understand how this could be the right place to specify that specific vector widths are legal and others aren't.
I think this must go in target-specific code.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:216-224
+    unsigned Op = MI.getOperand(i).getReg();
+    // For G_MERGE_VALUES and G_UNMERGE_VALUES, get the last operand
+    // if TypeIdx == 1 as they have variable number of operands.
+    if ((MI.getOpcode() == TargetOpcode::G_MERGE_VALUES ||
+         MI.getOpcode() == TargetOpcode::G_UNMERGE_VALUES) &&
+        TypeIdx == 1) {
+      Op = MI.getOperand(MI.getNumOperands() - 1).getReg();
----------------
This is very generic code, and having explicit checks for specific Gopcodes feels wrong.
Should the if test instead of reading
```
if ((MI.getOpcode() == TargetOpcode::G_MERGE_VALUES ||
         MI.getOpcode() == TargetOpcode::G_UNMERGE_VALUES) &&
        TypeIdx == 1) 
```
be more something like the following?
With some of the helper functions maybe still needing to be written:

```
if (hasVariableNumberOfOperands(MI.getOpcode() && TypeIdx == numberOfTypeIndices(MI.getOpcode())
```

Also, I fail to understand the need for this logic here with the little documentation that exists about G_MERGE_VALUES and G_UNMERGE_VALUES in include/llvm/Target/GenericOpcodes.td.
Are all of the operands in the variable-number-of-operands in G_MERGE_VALUES of the same type?
If so why do you need to get the Type of the last operand, instead of just any other operand?
If not, how can picking an action based on the type of the last operand by correct for the other operands that are of a different type?
It probably shows that I'm pretty confused here. I think I'd need quite a bit of explanation starting from exactly what constraints exist on G_(UN)MERGE_VALUES operands, the relationship with the TypeIdx, and going from there, maybe an example of what issue this code is trying to solve?


https://reviews.llvm.org/D39823