[PATCH] D74803: TableGen: Fix logic for default operands

Wed Feb 19 08:05:17 PST 2020

arsenm added a comment.

In D74803#1882699 <https://reviews.llvm.org/D74803#1882699>, @simon_tatham wrote:

> I might as well admit my ignorance up front: despite having been the last person to touch this code, I don't really understand what the distinction is between a `CodeGenInstruction` and a `DAGInstruction` here, apart from the fact that one of them has an operand list that includes the outputs as well as the inputs.
>
> As far as I can see, each `DAGInstruction` is built from a `CodeGenInstruction` by the loop in `CodeGenDAGPatterns::ParseInstructions()`, and it //looks// to me from reading that code as if the number of operands of the `DAGInstruction` should always be the same as `NumFixedOperands - NumResults` for the corresponding `CodeGenInstruction`, and the operand list is a copy of the same list of records. So without a test case or an existing example, I can't see how this change is making any difference at all.

The DAGInstruction is the pattern instruction, which may be missing the defaulted operands in a source pattern. The CodeGenInstruction is the target MachineInstr concept.

> I wrote this default-operand checking to use with the MVE instructions in the Arm backend. Certainly in that context it seems to make no difference which of these pieces of code is used: an instruction like `MVE_VADDi8`, for example, has three input operands in both of these views, with the last one being defaultable (even though it then consists of multiple sub-operands that show up as separate children of the DAG node).
> 
> Do you have an example of a case in which the code you're replacing gives different answers before and after this patch?

I get incorrect type inference failures in D74832 <https://reviews.llvm.org/D74832> without this patch since it ends up trying to merge the type of a non-default operand in with a default

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74803/new/

https://reviews.llvm.org/D74803