This is an archive of the discontinued LLVM Phabricator instance.

[MC] Store target Insts table in reverse order. NFC.
ClosedPublic

Authored by foad on Jan 20 2023, 7:15 AM.

Details

Summary

This will allow an entry in the table to access data that is stored
immediately after the end of the table, by adding its opcode value
to its address.

Diff Detail

Event Timeline

foad created this revision.Jan 20 2023, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 7:15 AM
foad requested review of this revision.Jan 20 2023, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 7:15 AM
arsenm added a subscriber: arsenm.Jan 20 2023, 7:44 AM
arsenm added inline comments.
llvm/include/llvm/MC/MCInstrInfo.h
65–66

Can the -1 be avoided?

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
2506–2508

Why can't this one use the usual MCInstrInfo::get?

foad added inline comments.
llvm/include/llvm/MC/MCInstrInfo.h
65–66

Yes, MCInstrInfo could store Desc + NumOpcodes - 1 instead of just Desc.

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
2506–2508

This is a method of ARMOperand, and it seems these addXXXOperands methods do not have access to the ARMAsmParser. I wanted to raise this with @simon_tatham who added this code in D62669. I wonder if this functionality could be reimplemented in an AsmMatchConverter instead. But I don't know if that is possible because I don't understand what this code is doing.

foad updated this revision to Diff 490876.Jan 20 2023, 8:35 AM

Store pointer to last entry in table for simpler indexing.

simon_tatham added inline comments.Jan 20 2023, 8:40 AM
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
2506–2508

And I don't know what an AsmMatchConverter can or can't do, so we may have a problem on both sides here :-)

There's a long explanation of the vpred_n and vpred_r operand classes in a comment in ARMInstrFormats.td. The general idea is that many MVE vector instructions which compute a value and store it in a vector register also come in a predicated form, in which the P0 predicate register indicates which lanes of the output register should be written with the results of the computation, and which should retain whatever data they contained before the instruction. To model this in a MachineInstr in a way that shows the data flow, it's necessary to provide the previous value of the output register as an input, and then tie them to each other so that register allocation will make them the same physical register.

So, at the MC layer, the upshot is that this input operand has to contain the same physical register number as the output operand it's tied to. But this predication system applies to many instructions, with different arrangements of other operands. So this code searches the instruction description to find the tie, in order to find the output operand that already contains the right register number, so as to copy it to the matching input operand.

simon_tatham added inline comments.Jan 20 2023, 9:09 AM
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
2506–2508

Here's a concrete example. If you run llvm-mc -triple thumbv8.1m.main -mattr=+mve -show-inst and feed it these instructions:

vadd.i16 q0, q1, q2
vpst
vaddt.i16 q0, q1, q2

then you get this output:

vadd.i16        q0, q1, q2      @ <MCInst #982 MVE_VADDi16
                                @  <MCOperand Reg:57>
                                @  <MCOperand Reg:58>
                                @  <MCOperand Reg:59>
                                @  <MCOperand Imm:0>
                                @  <MCOperand Reg:0>
                                @  <MCOperand Reg:0>
                                @  <MCOperand Reg:0>>
vpst                            @ <MCInst #1383 MVE_VPST
                                @  <MCOperand Imm:8>>
vaddt.i16       q0, q1, q2      @ <MCInst #982 MVE_VADDi16
                                @  <MCOperand Reg:57>
                                @  <MCOperand Reg:58>
                                @  <MCOperand Reg:59>
                                @  <MCOperand Imm:1>
                                @  <MCOperand Reg:56>
                                @  <MCOperand Reg:0>
                                @  <MCOperand Reg:57>>

The first three operands of each instruction are the obvious ones: 57 is q0, the output register, and 58 and 59 are q1 and q2, the explicit input registers. After that, the predicated VADD has an immediate 1 (indicating that it's predicated in "then" rather than "else" mode, i.e. writing the lanes corresponding to set bits of P0); then register 56 is the P0 predicate register; the empty Reg:0 parameter is only used for tail predication which we're not doing here; and finally Reg:57 is a second copy of q0, this time used as an input register. And the unpredicated version has 0,0,0,0 for those four operands.

foad added inline comments.Jan 20 2023, 9:18 AM
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
2506–2508

To model this in a MachineInstr in a way that shows the data flow, it's necessary to provide the previous value of the output register as an input, and then tie them to each other so that register allocation will make them the same physical register.

I'm pretty sure there are loads of examples like that in the AMDGPU backend, and we handle them in "convert" functions. But it's not my area of expertise and I have no idea how easy it would be to change the ARM backend over to that way of doing things.

barannikov88 added inline comments.
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
2506–2508

It should be easy to add MII to CreateVPTPred and VCCOp and then use it in addVPTPredROperands.
But I don't think it should be done in this patch.

barannikov88 accepted this revision.Jan 23 2023, 1:13 PM

LGTM

llvm/utils/TableGen/InstrInfoEmitter.cpp
935

nit: reverse should be visible without llvm:: qualification.

This revision is now accepted and ready to land.Jan 23 2023, 1:13 PM
This revision was landed with ongoing or failed builds.Jan 24 2023, 1:42 PM
This revision was automatically updated to reflect the committed changes.
foad marked an inline comment as done.Jan 24 2023, 1:42 PM