[PATCH] TableGen/MCInstPrinter: Generate a method to print complete machine Operands using the Target's PrintMethods

[Quentin Colombet]

Hi Ahmed,

Basically, there are two things to decide first:
- Should this be exposed at the MCInstPrinter level?
- Should this be optional?

See the details below.

On Nov 11, 2013, at 1:43 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:

> This patch generates an MCInstPrinter method to print arbitrary
> operands without needing to know the specific PrintMethod associated
> with the Operand, and using only the Operand type (as defined by the
> other patch).
> The method is optional, so I added the necessary bits to have it in X86 and ARM.
Do we actually need to expose this interface at the MCInstPrinter level?
Indeed, other methods like this were so far pushed into the XXXInstPrinter classes.

Note: I am not saying this is wrong, I would just like to understand why we are making something different here and why is it right.

> 
> Suggestions are obviously very welcome, especially on 1) the name,
The name is fine, IMHO.

> 2)
> whether to keep it #ifdef'd and optional or to make it pure virtual in
> MCInstPrinter.
I really do not like this #ifdef’d but if we want to keep it optional I guess we have not much choice.
The question is why would we want to keep it optional?

I only see two reasons for this being optional:
1. The size of the compiler executable, but I doubt this will have a significant impact.
2. This will break out-of-tree targets until they add the overloaded prototype, thus this is a straight forward fix.

Right now, there is nothing that uses this method, so I am fine for keeping it optional with the current. When we will contribute users of this method, we may change this scheme.

Anyway, If we really need to expose this interface at the MCInstPrinter level, I suggest we keep the default implementation in the MCInstPrinter with the llvm_unreachable (like in your patch), but with a different message in it. Indeed, we should convey the message that this method is autogenerated by tablegen and the target should override it to take advantage of it.
If we keep the method optional, explaining how to do that in a few words becomes harder, but we may point out to the entry the patch adds in the documentation.

 ** Comments on the code **
utils/TableGen/AsmWriterEmitter.cpp

#1.
+    "void " << TGName << ClassName
+            << "::printMachineOperand(const MCInst *MI, "
+            << "unsigned OpType, unsigned OpNo, raw_ostream &O) {\n";

If you have a MI + a OpNo, I guess you can get the OpType, can’t we?
If yes, the OpType argument becomes useless.
If no, how do we get the OpType in the first place to be able to call that function?

#2.
+          if (OpRec->isSubClassOf("Operand") && !OpRec->isAnonymous())
+            FoundOperands.insert(OpRec);
[…]
+  O << "  switch(OpType) {\n";
+  O << "  default:\n"
+    << "    llvm_unreachable(\"Trying to print invalid operand type!\");\n"
+    << "    break;\n";

What happens if someone calls the printMachineOperand with an anonymous operand?
It seems it will reach the unreachable statement.
Shouldn’t we have a softer behavior for anonymous operand, like printing anonymous?

Something maybe more related to your previous patch proposal, what happens if the OpType is a “regular” type, i.e., not a derived type?
Do not know if it can happen, though.

Cheers,
-Quentin

> 
> Thanks again,
> - Ahmed
> 
> ---
> docs/WritingAnLLVMBackend.rst                      | 25 ++++++++
> include/llvm/MC/MCInstPrinter.h                    |  5 ++
> lib/MC/MCInstPrinter.cpp                           |  5 ++
> lib/Target/ARM/InstPrinter/ARMInstPrinter.cpp      |  1 +
> lib/Target/ARM/InstPrinter/ARMInstPrinter.h        |  2 +
> lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.h      |  1 +
> lib/Target/X86/InstPrinter/X86ATTInstPrinter.cpp   |  1 +
> lib/Target/X86/InstPrinter/X86ATTInstPrinter.h     |  2 +
> lib/Target/X86/InstPrinter/X86IntelInstPrinter.cpp |  1 +
> lib/Target/X86/InstPrinter/X86IntelInstPrinter.h   |  2 +
> lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h      |  1 +
> utils/TableGen/AsmWriterEmitter.cpp                | 68 ++++++++++++++++++++++
> 12 files changed, 114 insertions(+)
> <0002-TableGen-Generate-a-printMachineOperand-method-takin.patch>