[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: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();
----------------
dsanders wrote:
> 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.
> 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.

Right, it seems like I was trying to get the last operand for G_MERGE_VALUES for no reason. I'll remove the special case for G_MERGE_VALUES.

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

I think it would not work because $src1 doesn't have to be the same type with variable_ops. `MRI.getType(MI.getOperand(LastTypeIdx).getReg())` must return type2.






https://reviews.llvm.org/D39823