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

Wed Feb 19 08:42:09 PST 2020

simon_tatham accepted this revision.
simon_tatham added a comment.
This revision is now accepted and ready to land.

In D74803#1882722 <https://reviews.llvm.org/D74803#1882722>, @arsenm wrote:

> 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

I'm completely confused by that patch: I tried to apply it locally in order to see your inference failure for myself, but I couldn't, because of conflicts. For example, it removes a declaration of `selectVOP3PMods0` from `AMDGPUInstructionSelector.h`, but as far as `git log` can tell me, that declaration has never existed in that header file at all, as of current master.

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

OK, thanks; now I see what I missed there – `CodeGen::parseInstructionPattern` can also create instances of `DAGInstruction`, and it creates the //interesting// ones, whereas `ParseInstructions()` just makes the trivial ones. But `parseInstructionPattern` does the creation using an `emplace`, without mentioning the class name, so my crude grep didn't find it...

Now I understand how this didn't cause a problem in MVE, because in the MVE instruction definitions, we generally didn't bother filling in the pattern fields in the `Instruction` records themselves. All the instructions with a defaulted operand are predicated, so they need more than one pattern anyway for unpredicated and predicated instructions, so it was easier to put them all in separate `Pat` records. So that's why the operand lists in `Inst` and `InstInfo` look identical in our case.

In that case, this patch looks sensible to me, although I have just proved that I'm a long way from being an expert in this area!

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

https://reviews.llvm.org/D74803