Skip to content

PowerPC: restore fcmpo instruction definition - #3052

Open
StilesCrisis wants to merge 3 commits into
capstone-engine:v5from
StilesCrisis:fix-ppc-fcmpo
Open

PowerPC: restore fcmpo instruction definition#3052
StilesCrisis wants to merge 3 commits into
capstone-engine:v5from
StilesCrisis:fix-ppc-fcmpo

Conversation

@StilesCrisis

Copy link
Copy Markdown

Problem

PowerPC's fcmpo (ordered floating-point compare) has no decoder table entry at all on this branch. Real fcmpo instructions fail to disassemble entirely - cs_disasm() returns 0 instructions, not "decoded but unnamed".

from capstone import *
CODE = bytes.fromhex('fc1c0040')  # real `fcmpo cr0, f28, f0` from a retail GameCube binary
md = Cs(CS_ARCH_PPC, CS_MODE_BIG_ENDIAN + CS_MODE_32)
print(len(list(md.disasm(CODE, 0x1000))))  # 0 - should be 1

Filed as #3051 with the initial (incomplete) root-cause finding; this PR is the actual fix.

Root cause

FCMPO was written correctly in suite/synctools/tablegen/PPC/PPCInstrInfo.td (the vendored snapshot this branch's PPC tables are generated from), then commented out with stale pre-refactor type names (CRRC/FPRC instead of the correct crrc/f4rc/f8rc), and never fixed when FCMPU was later split into FCMPUS/FCMPUD immediately below it - fcmpo should have gotten the identical single/double-precision-interpretation split at the same time, but the commented-out block was just never revisited.

Fix

Restore FCMPO using the exact pattern of the FCMPU split right below it (FCMPOS/FCMPOD, same Interpretation64Bit/isCodeGenOnly convention). Confirmed correct against real upstream LLVM 14.0.5's PPCInstrInfo.td, which carries this identical split, active and uncommented.

Also required: PPCScheduleP9.td's P9Model no longer claims CompleteModel, since the restored FCMPOS/FCMPOD have no P9 scheduling entry in this vendored snapshot (confirmed via an actual tblgen error without this change - 'P9Model' lacks information for 'FCMPOD'/'FCMPOS'). Timing data isn't used for disassembler-table generation.

A second, prerequisite fix

Regenerating PPC's tables at all - independent of fcmpo - is blocked by an unrelated, pre-existing bug: PPCInstrHTM.td's TBEGIN_RET references a PPCCustomInserterPseudo class that has never been defined anywhere in this vendored tree, since HTM support was added in 2019 (confirmed via full history grep across every commit touching suite/synctools/tablegen/PPC). PPCInstrInfo.td unconditionally includes PPCInstrHTM.td, so this breaks tblgen for the whole PPC target, not just HTM.

This has evidently been worked around locally and never upstreamed: capstone-engine/capstone#1898 (the PR that produced this branch's currently-shipped PPC tables) posts this exact fix as a local, uncommitted patch, needed just to get tblgen to run. TBEGIN_RET doesn't need a dedicated class in the first place - it already sits inside the same let hasSideEffects = 1, usesCustomInserter = 1 in { } scope as TCHECK_RET immediately above it, which already uses plain Pseudo.

Toolchain

That same PR thread is also where the real regen toolchain for this vendored tree is documented, in the contributors' own words - not the modern LLVM this repo's shared headers were later bumped to for other architectures. The PR author states directly: "I am using LLVM 7.0.1 (without patches)". A maintainer's question in the same thread ("still 8.0.0?") independently confirms the project's working vintage at the time was nowhere near a modern LLVM.

The regenerated .inc files in this PR were produced with an unpatched LLVM 7.0.1 llvm-tblgen, run through this branch's own suite/synctools converters.

Regenerated tables

PPCGenAsmWriter.inc, PPCGenDisassemblerTables.inc, PPCGenInstrInfo.inc, PPCMappingInsn.inc, and PPCMappingInsnName.inc are regenerated and included. PPCGenRegisterInfo.inc and PPCGenSubtargetInfo.inc are unchanged. PPCDisassembler.c, PPCInstPrinter.c, and PPCMapping.c (hand-maintained, not generated) are unchanged - no decoder/printer logic needed touching.

The three large .inc files show numeric churn well beyond the fcmpo insertion point. This is expected, not scope creep: TableGen's DecoderEmitter and mnemonic-index numbering are reassigned fresh on every regen and aren't stable across invocations, so adding one instruction shifts internal indices for everything generated after it in table order.

Verification

Built and tested against the real library API (cs_disasm/cs_op_count, not just inspecting generated source):

  • fcmpo decodes with the correct mnemonic and operands.
  • Spot-checked other instructions across the regenerated tables (lwz, stfs, cmpwi, addc, mulhd., fcmpu, crand) still decode correctly - no regressions from the regen.
  • Full capstone test suite: 25/25 passing.

TBEGIN_RET referenced a PPCCustomInserterPseudo class that has never
been defined anywhere in this vendored tablegen tree, since the day
HTM support was added (commit 839c5e2, 2019) - confirmed via full
history grep across every commit touching suite/synctools/tablegen/PPC.
Without this fix, tblgen cannot parse PPC.td at all, since
PPCInstrInfo.td unconditionally includes PPCInstrHTM.td.

This has evidently been worked around locally and never upstreamed:
see the PPC PS-support PR discussion (capstone-engine#1898,
comment by @riptl), which posts this exact one-line fix as a local,
uncommitted patch needed to regenerate PPC's tables at all.

TBEGIN_RET doesn't need a dedicated custom-inserter-pseudo class in
the first place - it already sits inside the same
`let hasSideEffects = 1, usesCustomInserter = 1 in { }` scope as
TCHECK_RET immediately above it, which already uses plain Pseudo.
FCMPO was written correctly in this vendored .td snapshot, then
commented out with stale pre-refactor type names (CRRC/FPRC instead
of crrc/f4rc/f8rc), and never fixed when FCMPU was later split into
FCMPUS/FCMPUD immediately below it - fcmpo should have gotten the
identical single/double-precision-interpretation split at the same
time, but the commented-out block was just never revisited.

Restore FCMPO using the exact pattern of the FCMPU split right below
it (FCMPOS/FCMPOD, same Interpretation64Bit/isCodeGenOnly convention).
Confirmed correct against real upstream LLVM 14.0.5's PPCInstrInfo.td,
which carries this identical split, active and uncommented.

FCMPOS/FCMPOD have no P9 scheduling entry in this snapshot, so
PPCScheduleP9's CompleteModel can no longer claim completeness -
verified empirically (tblgen: "'P9Model' lacks information for
'FCMPOD'"/'FCMPOS'" without this change). Timing data isn't used for
disassembler-table generation.
Regenerated via LLVM 7.0.1's llvm-tblgen (unpatched) plus this
branch's synctools converters. LLVM 7.0.1 was confirmed, via the
capstone-engine#1898 PR thread, to be the actual toolchain
used for this vendored PPC tree's last real regen (commit d6a7c36) -
not the much newer version this repo's shared headers were later
bumped to for other architectures.

PPCGenRegisterInfo.inc and PPCGenSubtargetInfo.inc are unchanged.
PPCDisassembler.c, PPCInstPrinter.c, and PPCMapping.c are unchanged -
no hand-maintained decoder/printer logic needed touching.

The three large .inc files show numeric churn well beyond the fcmpo
insertion point. This is expected, not scope creep: LLVM TableGen's
DecoderEmitter and mnemonic-index numbering are reassigned fresh on
every regen and are not stable across invocations, so adding one
instruction shifts internal indices for everything generated after
it in table order.

Verified via the real library API (cs_op_count/cs_disasm), not just
inspection: fcmpo now decodes with correct operands, and the full
capstone test suite passes (25/25).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant