Repository navigation
[rocjitsu] Add validated ISA additions - #10587
Conversation
|
It depends on #10431 |
github-actions
Bot
added
documentation
fae3c71 to
abfe14b
Compare
8f7ce37 to
17de8e6
Compare
abfe14b to
213f1d6
Compare
213f1d6 to
c578a7d
Compare
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
Pull request overview
Adds a validated “ISA additions” mechanism to the amdisa generator pipeline so repository-owned instruction definitions and encoding identifiers can be layered on top of the public MR ISA XML without modifying vendor snapshots, while preserving deterministic output and provenance.
Changes:
- Introduces
amdisa.isa_additionsto validate and atomically merge instruction/identifier additions, tracking provenance. - Integrates additions into
Parserand CLI (--isa-additions), including reachability validation against the final decode tables. - Refactors shared name-formatting helpers into
gpuisa.py, updates codegen usage, and adds extensive tests + documentation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| emulation/rocjitsu/lib/python/amdisa/tests/test_isa_additions.py | New end-to-end and unit tests for additions validation, provenance, reachability, and CLI behavior. |
| emulation/rocjitsu/lib/python/amdisa/parser.py | Applies additions prior to parsing; propagates provenance into Instruction; validates added-form decode reachability. |
| emulation/rocjitsu/lib/python/amdisa/isa_profile.py | Adds compatibility_instruction_slots to reserve synthesized opcode slots during additions validation. |
| emulation/rocjitsu/lib/python/amdisa/isa_additions.py | New module implementing additions schema/validation and atomic merge into the base XML tree. |
| emulation/rocjitsu/lib/python/amdisa/gpuisa.py | Centralizes formatting helpers and adds provenance types/fields (IsaAdditionProvenance, Instruction.source_addition). |
| emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py | Uses centralized opcode-name helpers from gpuisa. |
| emulation/rocjitsu/lib/python/amdisa/main.py | Adds --isa-additions and wires it into single- and multi-ISA flows. |
| emulation/rocjitsu/lib/python/amdisa/init.py | Exposes additions APIs/types at package level. |
| emulation/rocjitsu/docs/codegen.md | Documents additions file format, validation contract, and CLI usage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Public MR ISA snapshots may omit target-variant instructions and the encoding identifiers needed to decode them. Editing the checked-in base XML would obscure its provenance and make future snapshot updates harder to audit, while adding instructions alone can leave their opcodes unreachable from the decode table. Add a narrow, ordered ISA additions format for complete Instruction nodes and complete identifiers belonging to existing encodings. Validate base metadata, references, opcode ownership, identifier layout, collisions, and implied-literal relationships before merging the additions. Merge validated identifiers before normal encoding parsing so added instructions follow the existing decode-table path. Reject active instruction forms that remain unreachable, and retain additions provenance on IsaSpec and Instruction for later target-variant handling. Wire repeatable --isa-additions NAME:XML arguments into single- and multi-ISA generation. Keep the public MR ISA XML unchanged and preserve byte-identical generated output for empty additions documents. This change adds generator infrastructure only. It does not add gfx1251 instructions or modify generated runtime sources. Tests cover validation failures, ordered and atomic application, exact CDNA5 decode-table mapping, generated decoder dispatch, and byte-identical generation for empty additions documents. Signed-off-by: hanhanW <hanhan0912@gmail.com>
ISA additions could pass validation and fail later in parsing or code generation when an instruction node was incomplete, occupied a synthetic compatibility slot, or normalized to an existing C++ symbol. Modifier-bearing encodings could also reject valid identifiers because ownership was checked against the decoded base opcode. Validate complete MR-ISA instruction subtrees before merging them. Reserve compatibility instruction slots in ISA profiles, share generated-name formatting between validation and codegen, and use modifier-expanded opcodes for both ownership and reachability. Add focused regressions for malformed nodes, CDNA5 and RDNA4 slot collisions, CDNA1 MIMG OPM routing, and normalized-name collisions.
The additions reachability check assumed every internal provenance attribute was created by the current apply operation. A base XML carrying an unknown reserved attribute instead raised a bare KeyError and hid the malformed input. Check provenance IDs explicitly and report the XML path, instruction, and unknown value in an IsaAdditionError. Cover the failure with a CDNA5 parser regression that injects the reserved attribute into the base XML.
c4d57a4 to
277f694
Compare
277f694 to
659f5f0
Compare
|
Note that this no longer depends on any PR. |
atgutier
left a comment
atgutier
left a comment
There was a problem hiding this comment.
I have some agent-generated comments:
[MAJOR] /tmp/pr10587-head-659f5f0/emulation/rocjitsu/lib/python/amdisa/isa_additions.py:855 —
Distinct active encoding conditions generate the same instruction class twice
Reasoning: Repeated (encoding, opcode) forms are accepted whenever their condition strings differ.
For CDNA5 ENC_SOP1, both default and the normal non-literal condition are declared by the base XML
and treated as active by /tmp/pr10587-head-659f5f0/emulation/rocjitsu/lib/python/amdisa/
isa_profile.py:2443. Both forms therefore survive parsing and normalize to the same generated class.
Evidence: I reproduced this using an otherwise valid opcode-7 CDNA5 addition containing both
conditions. Parsing created two SAdditionTestB32Sop1 objects, and generation failed at /tmp/pr10587-
head-659f5f0/emulation/rocjitsu/lib/python/amdisa/codegen/_generator.py:410:
ValueError: duplicate split execution class: SAdditionTestB32Sop1
The symbol-collision check permits the duplicate because both symbols have the same instruction-name
owner. The existing test at /tmp/pr10587-head-659f5f0/emulation/rocjitsu/lib/python/amdisa/tests/
test_isa_additions.py:448 covers only a second condition that the profile skips.
Suggested fix: After profile filtering, require at most one active form for each generated
instruction symbol, or explicitly coalesce equivalent active forms. Add a CDNA5 regression using
both declared active SOP1 conditions.
Confidence: high; reproduced through exact-head parsing and generation.
[MAJOR] /tmp/pr10587-head-659f5f0/emulation/rocjitsu/lib/python/amdisa/isa_additions.py:899 —
Operand fields are not validated against the referenced encoding
Reasoning: Validation confirms only that each operand type exists. It never verifies that a field-
bearing operand names a field in the selected encoding or its applicable parent encoding.
The parser turns directly into the operand name at /tmp/pr10587-head-659f5f0/emulation/
rocjitsu/lib/python/amdisa/parser.py:1239. When that name is absent from the encoding layout, the
generator falls through to /tmp/pr10587-head-659f5f0/emulation/rocjitsu/lib/python/amdisa/codegen/
_generator.py:8970 and initializes the operand selector to zero.
Evidence: I changed the destination field of a cloned CDNA5 S_MOV_B32 addition from SDST to BOGUS.
Validation and generation both succeeded, producing:
bogus(32, OperandType::OPR_SDST, 0)
...
dst_operands_[0] = &bogus;
Every decoded instance would therefore report destination selector zero instead of reading the
encoded sdst bits.
Suggested fix: Record normalized microcode field names in _EncodingDefinition and reject unknown
field-bearing operands. Account explicitly for the established implied-literal operands that
intentionally do not appear in the effective parent layout. Add a mutation regression replacing SDST
with an unknown field and require rejection before tree mutation.
Confidence: high; reproduced through exact-head parsing and C++ generation.
MINOR
[MINOR] /tmp/pr10587-head-659f5f0/emulation/rocjitsu/lib/python/amdisa/isa_additions.py:850 —
Unknown encoding conditions silently discard additions
Reasoning: The condition text is passed directly to skip_inst_encoding() without checking that it
exists in the base encoding’s condition definitions. Most unknown non-default strings are
interpreted as inactive. /tmp/pr10587-head-659f5f0/emulation/rocjitsu/lib/python/amdisa/
parser.py:1215 consequently skips the instruction without an error.
Evidence: Changing a valid CDNA5 condition to the typo defualt succeeded and recorded the document
as applied, but produced zero instruction objects. Its opcode route remained decodeInvalid.
Suggested fix: Validate every condition against the referenced encoding’s declared condition names
or IDs before profile filtering. Add a typo-condition regression that expects IsaAdditionError and
verifies that the base tree remains unchanged.
Confidence: high; reproduced against the exact head.
[MINOR] /tmp/pr10587-head-659f5f0/emulation/rocjitsu/lib/python/amdisa/isa_additions.py:795 —
Invalid instruction names reach generated C++ identifiers
Reasoning: Instruction names need only be nonempty. /tmp/pr10587-head-659f5f0/emulation/rocjitsu/
lib/python/amdisa/gpuisa.py:70 removes underscores but otherwise preserves punctuation when
constructing class names.
Evidence: An otherwise valid addition named BAD-NAME passed validation and generated declarations
including:
class Bad-nameSop1
InstructionExecutionId::Bad-nameSop1
decodeBad-nameSop1
These are invalid C++ identifiers.
Suggested fix: Validate instruction names against the accepted MR-ISA mnemonic grammar before
collision checking, or use a validated injective formatter consistently. Add a rejected-name
regression using BAD-NAME.
Confidence: high; reproduced through exact-head generation.
Signed-off-by: hanhanW <hanhan0912@gmail.com>
hanhanW
left a comment
hanhanW
left a comment
There was a problem hiding this comment.
The review comments are addressed and corresponding tests are added.
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
Added some comments and a few inline replies
An addition could validate but emit no instruction when every form was removed by profile filtering. The implied-literal field exception also accepted a non-literal operand under a literal spelling, which could silently replace an encoded selector with zero. Compatibility slot ownership and injection duplicated the same instruction triples and could drift. Require every added instruction to retain an active form. Derive implied-literal field, type, and role contracts from base XML, while allowing new value sizes and data formats needed by target deltas. Make compatibility injectors obtain their encoding, opcode, and instruction name from the profile-owned slot mapping. Add atomic regressions for skipped forms and invalid literal roles, positive coverage for new literal value shapes, and multi-slot compatibility coverage. Update the additions documentation to state these contracts. Signed-off-by: hanhanW <hanhan0912@gmail.com>
TheRock Submodule Bump ActivityNewest first
|
Public MR ISA snapshots may omit target-variant instructions and the encoding identifiers needed to decode them. Editing the checked-in base XML would obscure its provenance and make future snapshot updates harder to audit, while adding instructions alone can leave their opcodes unreachable from the decode table.
Add a narrow, ordered ISA additions format for complete Instruction nodes and complete identifiers belonging to existing encodings. Validate base metadata, references, opcode ownership, identifier layout, collisions, and implied-literal relationships before merging the additions.
Merge validated identifiers before normal encoding parsing so added instructions follow the existing decode-table path. Reject active instruction forms that remain unreachable, and retain additions provenance on IsaSpec and Instruction for later target-variant handling.
Wire repeatable --isa-additions NAME:XML arguments into single- and multi-ISA generation. Keep the public MR ISA XML unchanged and preserve byte-identical generated output for empty additions documents.
This change adds generator infrastructure only. It does not add gfx1251 instructions or modify generated runtime sources.
Submission Checklist