[PATCH] D137653: [TableGen] More named sub-operands work.

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:6557
+  // the decoder doesn't actually call it yet. That will be addressed in a
+  // future change.)
+  return MCDisassembler::Success;
----------------
gbossu wrote:
> @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?
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137653