Page MenuHomePhabricator

[TableGen] Add "getOperandType" to get operand types from opcode/opidx
ClosedPublic

Authored by nlguillemot on Jun 14 2019, 12:47 AM.

Details

Summary

The InstrInfoEmitter outputs an enum called "OperandType" which gives numerical IDs to each operand type. This patch makes use of this enum to define a function called "getOperandType", which allows looking up the type of an operand given its opcode and operand index.

Diff Detail

Repository
rL LLVM

Event Timeline

nlguillemot created this revision.Jun 14 2019, 12:47 AM
nlguillemot updated this revision to Diff 204802.EditedJun 14 2019, 10:03 AM
  • Fix some syntax mistakes.
  • Map only records that derive from Operand and are not anonymous. This exactly matches the condition used to generate GET_INSTRINFO_OPERAND_TYPES_ENUM.

some syntax nits:
OpcodeOperandTypes [] -> OpcodeOperandTypes[] (removed unnecessary space)
OS << "-1"; -> OS << -1; (don't use string where not necessary

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.

nlguillemot added a comment.EditedJun 18 2019, 3:37 PM

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.

Indeed, I had the same question myself...

I couldn't find a particular way to use the enum otherwise myself, but I'm not an expert of the relevant code. MCOperandInfo does have a member that is somewhat misleadingly named OperandType, but that one seems to refer to llvm::MCOI::OperandType rather than this enum.

dsanders accepted this revision.Jul 12 2019, 2:46 PM

LGTM

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.

utils/TableGen/InstrInfoEmitter.cpp
356–357 ↗(On Diff #205433)

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

372 ↗(On Diff #205433)

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

397 ↗(On Diff #205433)

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.

404 ↗(On Diff #205433)

Whitespace nit after the //

This revision is now accepted and ready to land.Jul 12 2019, 2:46 PM

Could somebody do the commit for me? I don’t have commit access yet.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 3:10 PM