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

[Volkan Keles via Phabricator via llvm-commits]

volkan 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}});
----------------
kristof.beyls wrote:
> 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.
I thought about this, but then I decided to handle the common types here. My concern was targets use these operations in order to legalize some other operations. For example, in test/CodeGen/X86/GlobalISel/legalize-add.mir, there is a function called test_add_i64 as below.

```
bb.1 (%ir-block.0):
  %0(s64) = IMPLICIT_DEF
  %1(s64) = IMPLICIT_DEF
  %2(s64) = G_ADD %0, %1
  RET 0
```
Here, Legalizer builds G_MERGE_VALUES and G_UNMERGE_VALUES to extract 32-bit values, but they are not actually legal. The patch Aditya proposed (https://reviews.llvm.org/D39267) should fix this problem by processing the legalizer artifacts after legalizing all other instructions.

I'll move this to target-specific code and create another diff.


================
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();
----------------
kristof.beyls wrote:
> 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?
> Are all of the operands in the variable-number-of-operands in G_MERGE_VALUES of the same type?

Yes, G_MERGE_VALUES concatenates multiple registers of the same size into a wider register and G_UNMERGE_VALUES extracts multiple registers with the specified size. These instructions has variable number of operands, but there is only one source type and one destination type as all sources for G_MERGE_VALUES and all destination for G_UNMERGE_VALUES must be the same type.



> Should the if test instead of reading be more something like the following?

That would be incorrect because other instruction with variable number of operands may have other operands with different types. For example:

```
def G_OPCODE : Instruction {
  let OutOperandList = (outs type0:$dst);
  let InOperandList = (ins type1:$src0, type2:src1, ..., variable_ops);
}
```

So, this check is okay only for these instructions because they have size constraints.

> 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?

Simply, I'm trying to set legalizer actions for these instruction, but not for every operand. The existing definition for G_MERGE_VALUES is:

```
def G_MERGE_VALUES : Instruction {
  let OutOperandList = (outs type0:$dst);
  let InOperandList = (ins variable_ops);
  let hasSideEffects = 0;
}

```
In this case, there is no way to legalize source operands because `MI.getDesc().getNumOperands()` returns 1. We can fix this by replacing `MI.getDesc().getNumOperands()` with `MI.getNumOperands()`, but this means we need to set actions for every possible case.

I'll add a comment to explain why we are doing this.


https://reviews.llvm.org/D39823