[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: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:77
+                               {2, Unsupported},
+                               {4, Unsupported},
+                               {8, Legal},
----------------
Isn't this one redundant? We've already said 2<=Size<8 are unsupported


================
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();
----------------
volkan wrote:
> 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.
Do we really need to treat G_MERGE_VALUE's specially here? G_UNMERGE_VALUES needs it because type1 is only on the last operand, but for G_MERGE_VALUES, type1 is on operands 1 and higher.

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

So long as variable_ops is equivalent to specifying typeN (where N is 1 higher than the highest number used elsewhere in the definition) a variable number of times, I think Kristof's if-statement would work for your example. Either way, I agree with the main point that we should be driving this with a generic mechanism rather than a special case.
Also, it seems odd to me that we have type constraints that aren't described.

Maybe, we should have a variable_ops_typeN that indicates the type constraint and leaves some information in the MCInstrDesc we can inspect during this code.


https://reviews.llvm.org/D39823