Page MenuHomePhabricator

[TableGen] More named sub-operands work.
ClosedPublic

Authored by jyknight on Nov 8 2022, 9:27 AM.

Details

Summary

Commit a538d1f13a13 first added support for named sub-operands in
CodeEmitterGen. We now add a few more features to that, enabling
further target cleanups.

  1. Adds support for handling an EncoderMethod in a sub-operand in

CodeEmitterGen. Previously, the specified encoder of a sub-operand was
ignored, and only the default used.

  1. Adds support for sub-operands in DecoderEmitter, along with support

for tied sub-operands.

The changes to the decoder required a few minor tweaks to a few
targets, where existing brokeness was exposed. In order to keep this
patch small, I left FIXMEs which will be addressed in upcoming
patches. (Except MIPS16, since its object file emission/decoding is
totally broken).

Diff Detail

Event Timeline

jyknight created this revision.Nov 8 2022, 9:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 9:27 AM
jyknight requested review of this revision.Nov 8 2022, 9:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 9:27 AM
MaskRay added inline comments.Nov 13 2022, 12:03 AM
llvm/lib/Target/ARM/ARMInstrFormats.td
276

OK. This is to avoid lib/Target/ARM/ARMInstrCDE.td:530:5: error: DecoderEmitter: operand "vp" uses MIOperandInfo with multiple ops, but doesn't have a custom decoder!

llvm/utils/TableGen/DecoderEmitter.cpp
1909
2215–2216

The comment appears outdated/unclear even before this patch. Could you reword it?

2258

OpTypeRec->isSubClassOf("Operand") is worth a comment.

2295

Delete braces for the single simple statement

2299
2300

Add a test/TableGen test for this error if it isn't too difficult?

jyknight updated this revision to Diff 476780.Nov 20 2022, 5:43 PM
jyknight marked 7 inline comments as done.

Address review comments.

MaskRay accepted this revision.Dec 5 2022, 12:03 PM
This revision is now accepted and ready to land.Dec 5 2022, 12:03 PM
This revision was landed with ongoing or failed builds.Dec 7 2022, 11:37 AM
This revision was automatically updated to reflect the committed changes.
gbossu added a subscriber: gbossu.Tue, Jan 10, 8:27 AM
gbossu added inline comments.
llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
6557

@jyknight I actually had a local patch which I planned to upstream in the next days, but that was made before your changes. That patch introduces an additional bit DecodeZeroBitOperand = false; flag in DAGOperand to allow targets to still get decoding callbacks for operands, even if they are encoded with zero bits. It came with a few changes for targets which have these "zero-bit" operands, like AVR, AArch64 or RiscV if I remember correctly.

It kept the default behavior of ignoring "zero-bit" operands when DecodeZeroBitOperand is false though. IIRC AMDGPU relies on this mechanism to purposely skip operands. Should I push this patch forward, or will this be superseded by some of your upcoming work by essentially always generating disassembler callbacks?

jyknight added inline comments.Tue, Jan 17, 6:06 AM
llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
6557

Indeed I do have a work in progress change to fix this which I started back in November, but I haven't gotten back to yet. (I decided to complete the useDeprecatedPositionallyEncodedOperands cleanup first instead.)

I found that many targets do indeed currently "depend" on this behavior of ignoring no-bits operands -- but by-and-large only because they HAD to hardcode workarounds for the autogenerated decoder forgetting about them, in order to get correct behavior.

As with the other changes I've been making in this area, I intend to introduce the new behavior and control it with a Target option (e.g. useDeprecatedDecoderOperandSkipping), migrate all in-tree targets, and then delete the option.

gbossu added inline comments.Mon, Jan 23, 6:54 AM
llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
6557

Thanks for the answer, I believe introducing useDeprecatedDecoderOperandSkipping is a better solution, as it forces targets in the right direction. Unless you don't intend to continue with your work in progress, I guess it doesn't make sense for me to push my patch forward? Please let me know if I can somehow assist though, as I attempted a very similar change.