PowerPC: restore fcmpo instruction definition - #3052
Open
StilesCrisis wants to merge 3 commits into
Open
Conversation
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PowerPC's
fcmpo(ordered floating-point compare) has no decoder table entry at all on this branch. Realfcmpoinstructions fail to disassemble entirely -cs_disasm()returns 0 instructions, not "decoded but unnamed".Filed as #3051 with the initial (incomplete) root-cause finding; this PR is the actual fix.
Root cause
FCMPOwas written correctly insuite/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/FPRCinstead of the correctcrrc/f4rc/f8rc), and never fixed whenFCMPUwas later split intoFCMPUS/FCMPUDimmediately below it -fcmposhould have gotten the identical single/double-precision-interpretation split at the same time, but the commented-out block was just never revisited.Fix
Restore
FCMPOusing the exact pattern of theFCMPUsplit right below it (FCMPOS/FCMPOD, sameInterpretation64Bit/isCodeGenOnlyconvention). Confirmed correct against real upstream LLVM 14.0.5'sPPCInstrInfo.td, which carries this identical split, active and uncommented.Also required:
PPCScheduleP9.td'sP9Modelno longer claimsCompleteModel, since the restoredFCMPOS/FCMPODhave 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'sTBEGIN_RETreferences aPPCCustomInserterPseudoclass 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 touchingsuite/synctools/tablegen/PPC).PPCInstrInfo.tdunconditionally includesPPCInstrHTM.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_RETdoesn't need a dedicated class in the first place - it already sits inside the samelet hasSideEffects = 1, usesCustomInserter = 1 in { }scope asTCHECK_RETimmediately above it, which already uses plainPseudo.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
.incfiles in this PR were produced with an unpatched LLVM 7.0.1llvm-tblgen, run through this branch's ownsuite/synctoolsconverters.Regenerated tables
PPCGenAsmWriter.inc,PPCGenDisassemblerTables.inc,PPCGenInstrInfo.inc,PPCMappingInsn.inc, andPPCMappingInsnName.incare regenerated and included.PPCGenRegisterInfo.incandPPCGenSubtargetInfo.incare unchanged.PPCDisassembler.c,PPCInstPrinter.c, andPPCMapping.c(hand-maintained, not generated) are unchanged - no decoder/printer logic needed touching.The three large
.incfiles show numeric churn well beyond thefcmpoinsertion point. This is expected, not scope creep: TableGen'sDecoderEmitterand 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):fcmpodecodes with the correct mnemonic and operands.lwz,stfs,cmpwi,addc,mulhd.,fcmpu,crand) still decode correctly - no regressions from the regen.