[TableGen][Decoder] Decode operands with zero width or all bits known

[s-barannikov]

There are two classes of operands that DecoderEmitter cannot currently handle:

1. Operands that do not participate in instruction encoding.
2. Operands whose encoding contains no '?' bits (that is, only 0s and 1s).

Because of this, targets developed various workarounds. Some targets insert missing operands after an instruction has been (incompletely) decoded, other take into account the missing operands when printing the instruction. Some targets do neither of that and fail to correctly disassemble some instructions.

This patch makes it possible to decode both classes of operands and allows to remove post-decoding instruction adjustments.

For the case of operand with no contribution to instruction encoding, one should now add bits<0> OpName field to instruction encoding record. This will make DecoderEmitter generate a call to the decoder function specified by the operand's DecoderMethod. The function has a signature different from the usual one and looks like this:

static DecodeStatus DecodeImm42Operand(MCInst &Inst, const MCDisassembler *Decoder) {
  Inst.addOperand(MCOperand::createImm(42));
  return DecodeStatus::Success;
}

Notably, encoding bits are not passed to it (since there are none).

There is nothing special about the second case, the operand bits are passed as usual. The difference is that before this change, the function was not called if all the bits of the operand were known (no '?' in the operand encoding).

There are two options controlling the behavior. Passing an option enables the old behavior. They exist to allow smooth transition to the new behavior. They are temporary (yeah, I know) and will be removed once all targets migrate, possibly giving some more time to downstream targets.

Subsequent patches in the stack enable the new behavior on some in-tree targets.

[s-barannikov]

• [AArch64] Remove post-decoding instruction mutations #156364
• [AArch64] Provide a custom decoder for LDR_ZA/STR_ZA #156363
• [AArch64] Correctly disassemble TSB instruction #156362
• [AVR] Remove workarounds for instructions using Z register (NFCI) #156361
• [RISCV] Remove post-decoding instruction adjustments #156360
• [Hexagon] Remove post-decoding instruction adjustments #156359
• [TableGen][Decoder] Decode operands with zero width or all bits known #156358 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[s-barannikov]

Something went wrong with the graphite -_-
Sorry if that spammed someone

[jurahul]

Overall, the decoder emitter changes LGTM. I'll let other folks here chime in on the individual target changes.

[jurahul]

Would it also make sense to add this comment?

// Represents a span of bits in the instruction encoding that's based on a span
// of bits in an operand's encoding.
//
// Width is the width of the span.
// Base is the starting position of that span in the instruction encoding.
// Offset if the starting position of that span in the operand's encoding.
// That is, bits {Base + Width - 1, Base} in the instruction encoding form
// bits {Offset + Width - 1, Offset} in the operands encoding.
struct EncodingField {

If you want to incorporate it here, or I can create a separate PR as well.

[s-barannikov]

Would it also make sense to add this comment?

// Represents a span of bits in the instruction encoding that's based on a span
// of bits in an operand's encoding.
//
// Width is the width of the span.
// Base is the starting position of that span in the instruction encoding.
// Offset if the starting position of that span in the operand's encoding.
// That is, bits {Base + Width - 1, Base} in the instruction encoding form
// bits {Offset + Width - 1, Offset} in the operands encoding.
struct EncodingField {

If you want to incorporate it here, or I can create a separate PR as well.

Please create a separate PR and also borrow that comment we discussed so that this PR gets smaller after rebasing.

[jurahul]

Would it also make sense to add this comment?

// Represents a span of bits in the instruction encoding that's based on a span
// of bits in an operand's encoding.
//
// Width is the width of the span.
// Base is the starting position of that span in the instruction encoding.
// Offset if the starting position of that span in the operand's encoding.
// That is, bits {Base + Width - 1, Base} in the instruction encoding form
// bits {Offset + Width - 1, Offset} in the operands encoding.
struct EncodingField {

If you want to incorporate it here, or I can create a separate PR as well.

Please create a separate PR and also borrow that comment we discussed so that this PR gets smaller after rebasing.

Sounds good, will do,

[jurahul]

Borrowed the comment, if you want to revert that part.

[s-barannikov]

Can I get an approval here? I've extracted most of the target-specific changes into separate PRs, and the ones that remain should be trivial.

[llvm-ci]

LLVM Buildbot has detected a new failure on builder llvm-clang-aarch64-darwin running on doug-worker-4 while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/26729

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'lld :: MinGW/driver.test' FAILED ********************
Exit Code: 127

Command Output (stdout):
--
# RUN: at line 1
/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld -### foo.o -m i386pe 2>&1 | /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=X86 /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld '-###' foo.o -m i386pe
# note: command had no output on stdout or stderr
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=X86 /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# note: command had no output on stdout or stderr
# RUN: at line 7
echo "-### foo.o -m i386pe" > /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/tools/lld/test/MinGW/Output/driver.test.tmp.rsp
# executed command: echo '-### foo.o -m i386pe'
# note: command had no output on stdout or stderr
# RUN: at line 8
/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld @/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/tools/lld/test/MinGW/Output/driver.test.tmp.rsp 2>&1 | /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=X86 /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld @/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/tools/lld/test/MinGW/Output/driver.test.tmp.rsp
# note: command had no output on stdout or stderr
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=X86 /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# note: command had no output on stdout or stderr
# RUN: at line 10
/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld -### foo.o -m i386pep 2>&1 | /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=X64 /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld '-###' foo.o -m i386pep
# note: command had no output on stdout or stderr
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=X64 /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# note: command had no output on stdout or stderr
# RUN: at line 16
/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld -### foo.o -m thumb2pe 2>&1 | /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=ARM /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld '-###' foo.o -m thumb2pe
# note: command had no output on stdout or stderr
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=ARM /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# note: command had no output on stdout or stderr
# RUN: at line 22
/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld -### foo.o -m arm64pe 2>&1 | /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=ARM64 /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld '-###' foo.o -m arm64pe
# note: command had no output on stdout or stderr
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=ARM64 /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# note: command had no output on stdout or stderr
# RUN: at line 28
/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld -### foo.o -m arm64ecpe 2>&1 | /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=ARM64EC /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld '-###' foo.o -m arm64ecpe
# note: command had no output on stdout or stderr
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=ARM64EC /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# note: command had no output on stdout or stderr
# RUN: at line 34
/Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld -### foo.o -m arm64xpe 2>&1 | /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=ARM64X /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/ld.lld '-###' foo.o -m arm64xpe
# note: command had no output on stdout or stderr
# executed command: /Volumes/RAMDisk/buildbot-root/aarch64-darwin/build/bin/FileCheck -check-prefix=ARM64X /Users/buildbot/buildbot-root/llvm-project/lld/test/MinGW/driver.test
...

[llvm-ci]

LLVM Buildbot has detected a new failure on builder lldb-aarch64-windows running on linaro-armv8-windows-msvc-05 while building llvm at step 6 "test".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/11295

Here is the relevant piece of the build log for the reference

Step 6 (test) failure: build (failure)
...
PASS: lldb-api :: tools/lldb-dap/repl-mode/TestDAP_repl_mode_detection.py (1223 of 2300)
UNSUPPORTED: lldb-api :: tools/lldb-dap/restart/TestDAP_restart.py (1224 of 2300)
UNSUPPORTED: lldb-api :: tools/lldb-dap/restart/TestDAP_restart_console.py (1225 of 2300)
UNSUPPORTED: lldb-api :: tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py (1226 of 2300)
UNSUPPORTED: lldb-api :: tools/lldb-dap/save-core/TestDAP_save_core.py (1227 of 2300)
UNSUPPORTED: lldb-api :: tools/lldb-dap/send-event/TestDAP_sendEvent.py (1228 of 2300)
UNSUPPORTED: lldb-api :: tools/lldb-dap/server/TestDAP_server.py (1229 of 2300)
UNSUPPORTED: lldb-api :: tools/lldb-dap/source/TestDAP_source.py (1230 of 2300)
UNSUPPORTED: lldb-api :: tools/lldb-dap/stackTrace-x86/TestDAP_source_x86.py (1231 of 2300)
UNRESOLVED: lldb-api :: tools/lldb-dap/launch/TestDAP_launch.py (1232 of 2300)
******************** TEST 'lldb-api :: tools/lldb-dap/launch/TestDAP_launch.py' FAILED ********************
Script:
--
C:/Users/tcwg/scoop/apps/python/current/python.exe C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/llvm-project/lldb\test\API\dotest.py -u CXXFLAGS -u CFLAGS --env LLVM_LIBS_DIR=C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./lib --env LLVM_INCLUDE_DIR=C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/include --env LLVM_TOOLS_DIR=C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./bin --arch aarch64 --build-dir C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/lldb-test-build.noindex --lldb-module-cache-dir C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/lldb-test-build.noindex/module-cache-lldb\lldb-api --clang-module-cache-dir C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/lldb-test-build.noindex/module-cache-clang\lldb-api --executable C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./bin/lldb.exe --compiler C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./bin/clang.exe --dsymutil C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./bin/dsymutil.exe --make C:/Users/tcwg/scoop/shims/make.exe --llvm-tools-dir C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./bin --lldb-obj-root C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/tools/lldb --lldb-libs-dir C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./lib --cmake-build-type Release --skip-category=watchpoint C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\test\API\tools\lldb-dap\launch -p TestDAP_launch.py
--
Exit Code: 1

Command Output (stdout):
--
lldb version 22.0.0git (https://github.com/llvm/llvm-project.git revision ed3597e2f7c4a5ae1ad70de81032f2addf78e267)
  clang revision ed3597e2f7c4a5ae1ad70de81032f2addf78e267
  llvm revision ed3597e2f7c4a5ae1ad70de81032f2addf78e267
Skipping the following test categories: ['watchpoint', 'libc++', 'libstdcxx', 'dwo', 'dsym', 'gmodules', 'debugserver', 'objc', 'fork', 'pexpect']


--
Command Output (stderr):
--
UNSUPPORTED: LLDB (C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\bin\clang.exe-aarch64) :: test_args (TestDAP_launch.TestDAP_launch.test_args) (skip on windows) 

UNSUPPORTED: LLDB (C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\bin\clang.exe-aarch64) :: test_commands (TestDAP_launch.TestDAP_launch.test_commands) (skipping due to the following parameter(s): architecture) 

UNSUPPORTED: LLDB (C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\bin\clang.exe-aarch64) :: test_cwd (TestDAP_launch.TestDAP_launch.test_cwd) (skip on windows) 

========= DEBUG ADAPTER PROTOCOL LOGS =========

1757011362.901615858 (stdio) --> {"command":"initialize","type":"request","arguments":{"adapterID":"lldb-native","clientID":"vscode","columnsStartAt1":true,"linesStartAt1":true,"locale":"en-us","pathFormat":"path","supportsRunInTerminalRequest":true,"supportsVariablePaging":true,"supportsVariableType":true,"supportsStartDebuggingRequest":true,"supportsProgressReporting":true,"$__lldb_sourceInitFile":false},"seq":1}

1757011362.901816130 (stdio) queued (command=initialize seq=1)

1757011362.912346601 (stdio) <-- {"body":{"$__lldb_version":"lldb version 22.0.0git (https://github.com/llvm/llvm-project.git revision ed3597e2f7c4a5ae1ad70de81032f2addf78e267)\n  clang revision ed3597e2f7c4a5ae1ad70de81032f2addf78e267\n  llvm revision ed3597e2f7c4a5ae1ad70de81032f2addf78e267","completionTriggerCharacters":["."," ","\t"],"exceptionBreakpointFilters":[{"description":"C++ Catch","filter":"cpp_catch","label":"C++ Catch","supportsCondition":true},{"description":"C++ Throw","filter":"cpp_throw","label":"C++ Throw","supportsCondition":true},{"description":"Objective-C Catch","filter":"objc_catch","label":"Objective-C Catch","supportsCondition":true},{"description":"Objective-C Throw","filter":"objc_throw","label":"Objective-C Throw","supportsCondition":true}],"supportTerminateDebuggee":true,"supportsBreakpointLocationsRequest":true,"supportsCancelRequest":true,"supportsCompletionsRequest":true,"supportsConditionalBreakpoints":true,"supportsConfigurationDoneRequest":true,"supportsDataBreakpoints":true,"supportsDelayedStackTraceLoading":true,"supportsDisassembleRequest":true,"supportsEvaluateForHovers":true,"supportsExceptionFilterOptions":true,"supportsExceptionInfoRequest":true,"supportsFunctionBreakpoints":true,"supportsHitConditionalBreakpoints":true,"supportsInstructionBreakpoints":true,"supportsLogPoints":true,"supportsModuleSymbolsRequest":true,"supportsModulesRequest":true,"supportsReadMemoryRequest":true,"supportsSetVariable":true,"supportsSteppingGranularity":true,"supportsValueFormattingOptions":true,"supportsWriteMemoryRequest":true},"command":"initialize","request_seq":1,"seq":0,"success":true,"type":"response"}

1757011362.913207293 (stdio) --> {"command":"launch","type":"request","arguments":{"program":"C:\\Users\\tcwg\\llvm-worker\\lldb-aarch64-windows\\build\\lldb-test-build.noindex\\tools\\lldb-dap\\launch\\TestDAP_launch.test_debuggerRoot\\a.out","initCommands":["settings clear --all","settings set symbols.enable-external-lookup false","settings set target.inherit-tcc true","settings set target.disable-aslr false","settings set target.detach-on-error false","settings set target.auto-apply-fixits false","settings set plugin.process.gdb-remote.packet-timeout 60","settings set symbols.clang-modules-cache-path \"C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/lldb-test-build.noindex/module-cache-lldb\\lldb-api\"","settings set use-color false","settings set show-statusline false","settings set target.env-vars PATH=","platform shell echo cwd = %cd%"],"debuggerRoot":"C:\\Users\\tcwg\\llvm-worker\\lldb-aarch64-windows\\build\\lldb-test-build.noindex\\tools\\lldb-dap\\launch","disableASLR":false,"enableAutoVariableSummaries":false,"enableSyntheticChildDebugging":false,"displayExtendedBacktrace":false},"seq":2}

1757011362.913281202 (stdio) queued (command=launch seq=2)

1757011362.971207857 (stdio) <-- {"body":{"category":"console","output":"Running initCommands:\n"},"event":"output","seq":0,"type":"event"}

1757011362.971273184 (stdio) <-- {"body":{"category":"console","output":"(lldb) settings clear --all\n"},"event":"output","seq":0,"type":"event"}

[DragonDisciple]

Hi @s-barannikov

Our team maintains a downstream target that's running into an interesting issue with this commit. We had plenty of "hacks" for implicit operands, and those were relatively straightforward to update.

However, our decoder is now acting weirdly for a very specific construct:

• Operand is part of the "ins" dag for an instruction
• Operand is encoded in n bits
• Operand value is known statically to be all-zeroes

Something like:

def instr: Instruction<..., (outs), (ins OpTy:$op), ...> {
  bits<2> op = 0b00;
  let Instr{1-0} = op;
}

The decoder does not do anything that seems reasonable. Incoming to emitBinaryParser, we have:

• OpInfo.Fields.empty() == true, which I believe is weird. Isn't there a field that spans bits 1 and 0?
• OpInfo.InitValue.has_value() == true

This qualifies for a "fully defined operand", but does not trigger "UseInsertBits". Since there are no fields , the 'tmp' used to hold the decoded value for the field is never initialized. In the generated disassembler tables:

case 1:
    // NOTE: tmp is uninitialized
    if (!Check(S, DecodeOpTy(MI, tmp, Address, Decoder))) { return MCDisassembler::Fail; }
    return S;

So which would you surmise is the issue?:

• OpInfo.Fields should not be empty for 'op'
• There should be a check for OpInfo.Fields being empty but having InitValue.has_value() to ensure tmp is initialized

Edit/Update:
The quick fix I came up with. Seems reasonable to ensure the initial value is respected:

diff --git i/llvm/utils/TableGen/DecoderEmitter.cpp w/llvm/utils/TableGen/DecoderEmitter.cpp
index ebb7deb757c4..a1491e19ddfd 100644
--- i/llvm/utils/TableGen/DecoderEmitter.cpp
+++ w/llvm/utils/TableGen/DecoderEmitter.cpp
@@ -1142,7 +1142,7 @@ void DecoderTableBuilder::emitBinaryParser(raw_ostream &OS, indent Indent,
   // encoded sequentially or has a non-zero constant part in the encoding.
   bool UseInsertBits = OpInfo.numFields() > 1 || OpInfo.InitValue.value_or(0);
 
-  if (UseInsertBits) {
+  if (UseInsertBits || OpInfo.fields().empty()) {
     OS << Indent << "tmp = 0x";
     OS.write_hex(OpInfo.InitValue.value_or(0));
     OS << ";\n";

[s-barannikov]

Hi, @DragonDisciple

I had a few iterations on this and in the final iteration I forgot about the case of "all zeros init value" and didn't bother to add tests because I hoped that in-tree targets cover all cases. It appears I was wrong.

So which would you surmise is the issue?:

• OpInfo.Fields should not be empty for 'op'
• There should be a check for OpInfo.Fields being empty but having InitValue.has_value() to ensure tmp is initialized

The second.

Edit/Update:
The quick fix I came up with. Seems reasonable to ensure the initial value is respected:

The fix looks correct to me, I'll prepare a PR, thanks!