[PATCH] D63320: [TableGen] Add "getOperandType" to get operand types from opcode/opidx

[Daniel Sanders via Phabricator via llvm-commits]

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

LGTM

In D63320#1549309 <https://reviews.llvm.org/D63320#1549309>, @craig.topper wrote:

> Can the enum  be used without this? I'm trying to understand why the enum was there to begin with. Only one in tree target, AVR, defines GET_INSTRINFO_OPERAND_TYPES_ENUM. Its not built by default so I could't really check, but looking through the file that included it, I couldn't prove it was being used.


Enabling AVR and deleting the generated enum doesn't break anything so I don't think it's been used in-tree. I'm aware of two out-of-tree users (one of them being Nicolas :-)) and presumably AVR has one too as I'd be surprised if they added it without something that needed it given that nobody else has it.

One potential in-tree use I see for OpTypes is using it in TargetInstrInfo::verifyInstruction() to verify operand constraints without having to test the actual instruction. I suppose GISel Combiners could potentially use it to combine instructions while still honouring things like addressing-mode constraints.



================
Comment at: utils/TableGen/InstrInfoEmitter.cpp:356-357
+
+  OS << "#ifdef GET_INSTRINFO_OPERAND_TYPE\n";
+  OS << "#undef GET_INSTRINFO_OPERAND_TYPE\n";
+  OS << "namespace llvm {\n";
----------------
Given that you depend on GET_INSTRINFO_OPERAND_TYPES_ENUM, it may be worth moving this before GET_INSTRINFO_OPERAND_TYPES_ENUM and `#define`-ing it in this block. It doesn't matter too much though as it's easy enough to have the including source define them both


================
Comment at: utils/TableGen/InstrInfoEmitter.cpp:372
+
+        const DagInit *MIOI = Op.MIOperandInfo;
+        if (!MIOI || MIOI->getNumArgs() == 0) {
----------------
It would be nice if we could still see the multi-operand operand before expansion but we don't have a need for it so it's fine to leave it out for now


================
Comment at: utils/TableGen/InstrInfoEmitter.cpp:397
+    OS << "  };\n";
+    OS << "  return OpcodeOperandTypes[Opcode].begin()[OpIdx];\n";
+  } else {
----------------
Does it make sense to factor out instructions with the same list to compress the table? I expect the number of rows would drop quite a bit as ISA's generally have a lot of commonality between and/or/xor/etc. and similar.


================
Comment at: utils/TableGen/InstrInfoEmitter.cpp:404
+  OS << "} // end namespace llvm\n";
+  OS << "#endif //GET_INSTRINFO_OPERAND_TYPE\n\n";
 }
----------------
Whitespace nit after the `//`


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

https://reviews.llvm.org/D63320