apply_offset_sram opcode support - #344
Conversation
Signed-off-by: Brian Xu <brianx@xilinx.com>
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds ISA support for a new APPLY_OFFSET_SRAM opcode intended for LLM MoE workflows, updating the ISA spec and generated headers/docs accordingly.
Changes:
- Introduces
APPLY_OFFSET_SRAM (0x24)in the ISA YAML spec and documentation outputs (MD/HTML). - Updates generated opcode/opsizes in
isa_stubs.hand dispatch plumbing inisa_defines.h. - Removes the
patch_bufoperand type and the prior optionalpad_bufargument fromAPPLY_OFFSET_57.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/aie2ps/stubs.h | Updates copyright header in the stub template. |
| templates/aie2ps/defines.h | Updates copyright header in the defines template. |
| specification/aie2ps/isa_stubs.h | Adds new opcode/size defines for APPLY_OFFSET_SRAM. |
| specification/aie2ps/isa_defines.h | Adds new dispatch function and opcode case for APPLY_OFFSET_SRAM. |
| specification/aie2ps/isa-spec.yaml | Defines the new opcode and updates/removes operand type usage in the spec source. |
| specification/aie2ps/isa-spec.md | Reflects spec updates in Markdown output, including the new opcode section. |
| specification/aie2ps/isa-spec.html | Reflects spec updates in HTML output, including the new opcode section and updated TOC. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 16-bit unsigned integer containing the size of the job (`START_JOB` until `END_JOB`), automatically | ||
| calculated and filled by the assembler. | ||
| In human-readable representations (asm file), this operand is omitted from the operand list. | ||
| - name: patch_buf | ||
| description: | | ||
| Used for host address patching. Arg with this type has name starting with @, and this type of arg | ||
| is consumed only by assembler. | ||
| # Note: An operation is considered 'regular' in case it is an actual operation used within a job 'body'. | ||
| # For example, UC_DMA_WRITE_DES is regular while START_JOB, END_JOB and EOF are not. | ||
| operations: |
| - name: offset | ||
| type: const | ||
| width: 16 | ||
| - name: pad_buf | ||
| type: patch_buf | ||
| brief: Applies an offset to one or more shim DMA BD base address fields. (Version for 57-bit base addresses.) | ||
| description: | | ||
| Patches the base address of `num_entries` shim DMA buffer descriptors by adding the offset | ||
| loaded from 'offset' and 'offset+1' in argument list. | ||
| if the offset is 0xFFFF, the offset added is the host address of 1st page of control code | ||
| The location of the `num_entries` buffer descriptors should be given in a table stored | ||
| at `table_ptr`. One entry in the table is a set of shim DMA BDs. If there are multiple entries in the table, | ||
| those set of BDS have to be contiguous. | ||
| 4th optional arg `pad_buf` with special type `patch_buf`, specifies the pad buffer to hold blob for control packet, | ||
| or save/restore L2 as scratchpad. It is a string starting with @, and only consumed by assembler to do host address | ||
| patch at compile time. | ||
| Patches the address fields in shim DMA BDs by adding the value loaded global registers 'offset' and 'offset+1'. | ||
| If the offset is 0xFFFF, the offset added is the host address of 1st page of control code. | ||
| shim DMA BDs are saved in a table pointed by `table_ptr`. One entry in the table is a set of shim DMA BD. | ||
| `num_entries` specifies the number of shim DMA BDs in the table. |
| FORCE_INLINE_FOR_RELEASE_ONLY static inline unsigned int control_dispatch_apply_offset_sram(const uint8_t *pc) | ||
| { | ||
| return control_op_apply_offset_sram( | ||
| pc, | ||
| /* table_ptr (const) */ *(uint16_t *)(&pc[2]), | ||
| /* num_entries (const) */ *(uint16_t *)(&pc[4]), | ||
| /* address (const) */ *(uint32_t *)(&pc[8]) | ||
| ); | ||
| } |
| The patching using this operation needs be done _before_ programming the shim DMA buffer | ||
| descriptors via the uC-DMA. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ); | ||
| } | ||
|
|
||
| FORCE_INLINE_FOR_RELEASE_ONLY static inline unsigned int control_dispatch_apply_offset_sram(const uint8_t *pc) |
There was a problem hiding this comment.
warning: unknown type name 'FORCE_INLINE_FOR_RELEASE_ONLY' [clang-diagnostic-error]
FORCE_INLINE_FOR_RELEASE_ONLY static inline unsigned int control_dispatch_apply_offset_sram(const uint8_t *pc)
^|
|
||
| FORCE_INLINE_FOR_RELEASE_ONLY static inline unsigned int control_dispatch_apply_offset_sram(const uint8_t *pc) | ||
| { | ||
| return control_op_apply_offset_sram( |
There was a problem hiding this comment.
warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]
return control_op_apply_offset_sram(
^| { | ||
| return control_op_apply_offset_sram( | ||
| pc, | ||
| /* table_ptr (const) */ *(uint16_t *)(&pc[2]), |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
/* table_ptr (const) */ *(uint16_t *)(&pc[2]),
^| return control_op_apply_offset_sram( | ||
| pc, | ||
| /* table_ptr (const) */ *(uint16_t *)(&pc[2]), | ||
| /* num_entries (const) */ *(uint16_t *)(&pc[4]), |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
/* num_entries (const) */ *(uint16_t *)(&pc[4]),
^| pc, | ||
| /* table_ptr (const) */ *(uint16_t *)(&pc[2]), | ||
| /* num_entries (const) */ *(uint16_t *)(&pc[4]), | ||
| /* address (const) */ *(uint32_t *)(&pc[8]) |
There was a problem hiding this comment.
warning: 8 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
/* address (const) */ *(uint32_t *)(&pc[8])
^| pc, | ||
| /* table_ptr (const) */ *(uint16_t *)(&pc[2]), | ||
| /* num_entries (const) */ *(uint16_t *)(&pc[4]), | ||
| /* address (const) */ *(uint32_t *)(&pc[8]) |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
/* address (const) */ *(uint32_t *)(&pc[8])
^| ); | ||
| } | ||
|
|
||
| FORCE_INLINE_FOR_RELEASE_ONLY static inline unsigned int control_dispatch_add(const uint8_t *pc) |
There was a problem hiding this comment.
warning: unknown type name 'FORCE_INLINE_FOR_RELEASE_ONLY' [clang-diagnostic-error]
FORCE_INLINE_FOR_RELEASE_ONLY static inline unsigned int control_dispatch_add(const uint8_t *pc)
^| #define ISA_OPCODE_READ_32 0x0c | ||
| #define ISA_OPCODE_READ_32_D 0x0d | ||
| #define ISA_OPCODE_APPLY_OFFSET_57 0x0e | ||
| #define ISA_OPCODE_APPLY_OFFSET_SRAM 0x24 |
There was a problem hiding this comment.
warning: macro 'ISA_OPCODE_APPLY_OFFSET_SRAM' used to declare a constant; consider using a 'constexpr' constant [cppcoreguidelines-macro-usage]
#define ISA_OPCODE_APPLY_OFFSET_SRAM 0x24
^| #define ISA_OPSIZE_READ_32 0x08 | ||
| #define ISA_OPSIZE_READ_32_D 0x04 | ||
| #define ISA_OPSIZE_APPLY_OFFSET_57 0x08 | ||
| #define ISA_OPSIZE_APPLY_OFFSET_SRAM 0x0c |
There was a problem hiding this comment.
warning: macro 'ISA_OPSIZE_APPLY_OFFSET_SRAM' used to declare a constant; consider using a 'constexpr' constant [cppcoreguidelines-macro-usage]
#define ISA_OPSIZE_APPLY_OFFSET_SRAM 0x0c
^
Problem solved by the commit
add new apply_offset_sram opcode for LLM MoE support
https://amd.atlassian.net/wiki/spaces/AIE/pages/1818057241
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
How problem was solved, alternative solutions (if any) and why they were rejected
change isa-spec.yaml, and the template to create isa_stubs.h and isa_defines.h
Risks (if any) associated the changes in the commit
What has been tested and how, request additional testing if necessary
Documentation impact (if any)