load_pdi, load_cores, load_cores_cp, preempt, start_cond_job_preempt check#290
Conversation
| // Verify load_pdi opcode count is equal across all columns in multi-UC control code. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_load_pdi_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:118:
- " has " + std::to_string(got) + " load_pdi opcodes\n");
+ " has " + std::to_string(got) + " load_pdi opcodes\n");
+ }| // Verify load_cores opcode count is equal across all columns. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_load_cores_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:128:
- " has " + std::to_string(got) + " load_cores opcodes\n");
+ " has " + std::to_string(got) + " load_cores opcodes\n");
+ }| // Verify load_cores_cp opcode count is equal across all columns. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_load_cores_cp_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:138:
- " has " + std::to_string(got) + " load_cores_cp opcodes\n");
+ " has " + std::to_string(got) + " load_cores_cp opcodes\n");
+ }| // Verify start_cond_job_preempt opcode count is equal across all columns. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_start_cond_job_preempt_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:148:
- " has " + std::to_string(got) + " start_cond_job_preempt opcodes\n");
+ " has " + std::to_string(got) + " start_cond_job_preempt opcodes\n");
+ }| ? cdata.try_add_load_pdi_label(label_arg) | ||
| : cdata.try_add_load_cores_label(label_arg); | ||
| if (!inserted) | ||
| throw error(error::error_code::invalid_asm, |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| throw error(error::error_code::invalid_asm, | |
| rted) { |
src/cpp/preprocessor/asm/asm_parser.cpp:429:
- \n");
+ \n");
+ }There was a problem hiding this comment.
Pull request overview
This PR adds assembler/preprocessor validation for AIE4 control-code opcode restrictions around load_pdi, load_cores, load_cores_cp, preempt, and start_cond_job_preempt, and introduces negative integration tests (via aiebu-asm -t aie4_config) to ensure these restrictions are enforced.
Changes:
- Add per-column opcode counters and
@labeluniqueness tracking inasm_parser, plus verification helpers for cross-column count consistency andpreemptprerequisites. - Enforce new restrictions during parsing and during the AIE4/AIE2PS preprocessing pipeline with clear
invalid_asmerrors. - Add a new
test/aie4-ctrlcode/opcode_checkstest suite with WILL_FAIL tests covering each new restriction.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/aie4-ctrlcode/opcode_checks/start_cond_count_mismatch.asm | Negative asm to trigger cross-column start_cond_job_preempt count mismatch. |
| test/aie4-ctrlcode/opcode_checks/start_cond_before_preempt.asm | Negative asm to trigger start_cond_job_preempt appearing before any preempt. |
| test/aie4-ctrlcode/opcode_checks/preempt_no_load_pdi.asm | Negative asm to trigger preempt usage without any load_pdi. |
| test/aie4-ctrlcode/opcode_checks/load_pdi_count_mismatch.asm | Negative asm to trigger cross-column load_pdi count mismatch. |
| test/aie4-ctrlcode/opcode_checks/load_cores_cp_count_mismatch.asm | Negative asm to trigger cross-column load_cores_cp count mismatch. |
| test/aie4-ctrlcode/opcode_checks/load_cores_count_mismatch.asm | Negative asm to trigger cross-column load_cores count mismatch. |
| test/aie4-ctrlcode/opcode_checks/dup_load_pdi_addr.asm | Negative asm to trigger duplicate load_pdi @label usage within a column. |
| test/aie4-ctrlcode/opcode_checks/dup_load_cores_addr.asm | Negative asm to trigger duplicate load_cores @label usage within a column. |
| test/aie4-ctrlcode/opcode_checks/config_start_cond_count_mismatch.json | Config wrapper for start_cond_count_mismatch.asm. |
| test/aie4-ctrlcode/opcode_checks/config_start_cond_before_preempt.json | Config wrapper for start_cond_before_preempt.asm. |
| test/aie4-ctrlcode/opcode_checks/config_preempt_no_load_pdi.json | Config wrapper for preempt_no_load_pdi.asm. |
| test/aie4-ctrlcode/opcode_checks/config_load_pdi_count_mismatch.json | Config wrapper for load_pdi_count_mismatch.asm. |
| test/aie4-ctrlcode/opcode_checks/config_load_cores_cp_count_mismatch.json | Config wrapper for load_cores_cp_count_mismatch.asm. |
| test/aie4-ctrlcode/opcode_checks/config_load_cores_count_mismatch.json | Config wrapper for load_cores_count_mismatch.asm. |
| test/aie4-ctrlcode/opcode_checks/config_dup_load_pdi_addr.json | Config wrapper for dup_load_pdi_addr.asm. |
| test/aie4-ctrlcode/opcode_checks/config_dup_load_cores_addr.json | Config wrapper for dup_load_cores_addr.asm. |
| test/aie4-ctrlcode/opcode_checks/CMakeLists.txt | Adds new WILL_FAIL ctests for opcode restriction enforcement via aie4_config. |
| test/aie4-ctrlcode/CMakeLists.txt | Hooks the new opcode_checks subdirectory into the AIE4 ctrlcode test suite. |
| src/cpp/preprocessor/asm/asm_parser.h | Adds counters/sets for opcode checks and verification helpers; adds current_col() helper. |
| src/cpp/preprocessor/asm/asm_parser.cpp | Implements parse-time counting/uniqueness checks for new opcodes and uses current_col(). |
| src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h | Enforces new cross-column opcode count checks and preempt ⇒ load_pdi requirement during preprocessing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Extract the @label argument that identifies the PDI / core-elf host address. | ||
| // Assembly notation omits PAD args, so the actual args are: <id>, <@label> | ||
| // The @label (pdi_host_addr_offset / core_elf_host_addr_offset) is at index 1. | ||
| // Use direct string search instead of a stringstream to avoid a heap allocation. | ||
| std::string label_arg; | ||
| { | ||
| auto first_comma = arg_str.find(','); | ||
| if (first_comma != std::string::npos) { | ||
| auto second_comma = arg_str.find(',', first_comma + 1); | ||
| auto start = first_comma + 1; | ||
| auto len = (second_comma != std::string::npos) | ||
| ? second_comma - start | ||
| : std::string::npos; | ||
| label_arg = trim(arg_str.substr(start, len)); | ||
| } | ||
| } | ||
| // Enforce uniqueness of the PDI / core-elf address within this column. | ||
| if (!label_arg.empty()) { | ||
| bool inserted = is_load_pdi | ||
| ? cdata.try_add_load_pdi_label(label_arg) | ||
| : cdata.try_add_load_cores_label(label_arg); | ||
| if (!inserted) | ||
| throw error(error::error_code::invalid_asm, | ||
| op_name + " location '" + label_arg + "' is not unique in column " + | ||
| std::to_string(col) + "; each " + op_name + | ||
| " in a control code elf must use a distinct address\n"); |
| // Verify load_pdi opcode count is equal across all columns in multi-UC control code. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_load_pdi_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:118:
- " for col " + std::to_string(col) + "\n");
+ " for col " + std::to_string(col) + "\n");
+ }| // Verify load_cores opcode count is equal across all columns. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_load_cores_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:128:
- " for col " + std::to_string(col) + "\n");
+ " for col " + std::to_string(col) + "\n");
+ }| // Verify load_cores_cp opcode count is equal across all columns. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_load_cores_cp_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:138:
- " for col " + std::to_string(col) + "\n");
+ " for col " + std::to_string(col) + "\n");
+ }| // Verify start_cond_job_preempt opcode count is equal across all columns. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_start_cond_job_preempt_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:148:
- " for col " + std::to_string(col) + "\n");
+ " for col " + std::to_string(col) + "\n");
+ }| ? cdata.try_add_load_pdi_label(label_arg) | ||
| : cdata.try_add_load_cores_label(label_arg); | ||
| if (!inserted) | ||
| throw error(error::error_code::invalid_asm, |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| throw error(error::error_code::invalid_asm, | |
| rted) { |
src/cpp/preprocessor/asm/asm_parser.cpp:421:
- \n");
+ \n");
+ }
sonals
left a comment
There was a problem hiding this comment.
See the details:
- Some code refactoring for the existing and new rules check
- Message consistency
- uint32_t vs unsigned int
xuhz
left a comment
There was a problem hiding this comment.
one more check to add:
if there is load_cores or load_cores_cp, there must be load_pdi
i already updated the confluence page for this.
…check Signed-off-by: Himanshu Choudhary <Himanshu.Choudhary@amd.com>
Signed-off-by: Himanshu Choudhary <Himanshu.Choudhary@amd.com>
Signed-off-by: Himanshu Choudhary <Himanshu.Choudhary@amd.com>
cb4c3d2 to
8466808
Compare
added |
done.. except uint32_t vs unsigned int |
| // Verify load_pdi opcode count is equal across all columns in multi-UC control code. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_load_pdi_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:118:
- " for column " + std::to_string(col) + "\n");
+ " for column " + std::to_string(col) + "\n");
+ }| // Verify load_cores opcode count is equal across all columns. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_load_cores_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:135:
- " for column " + std::to_string(col) + "\n");
+ " for column " + std::to_string(col) + "\n");
+ }| // Verify load_cores_cp opcode count is equal across all columns. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_load_cores_cp_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:145:
- " for column " + std::to_string(col) + "\n");
+ " for column " + std::to_string(col) + "\n");
+ }| // Verify start_cond_job_preempt opcode count is equal across all columns. | ||
| { | ||
| auto [ok, exp, col, got] = parser->verify_start_cond_job_preempt_count(); | ||
| if (!ok) |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| if (!ok) | |
| if (!ok) { |
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h:155:
- " for column " + std::to_string(col) + "\n");
+ " for column " + std::to_string(col) + "\n");
+ }| ? cdata.try_add_load_pdi_label(label_arg) | ||
| : cdata.try_add_load_cores_label(label_arg); | ||
| if (!inserted) | ||
| throw error(error::error_code::invalid_asm, |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| throw error(error::error_code::invalid_asm, | |
| rted) { |
src/cpp/preprocessor/asm/asm_parser.cpp:315:
- n");
+ n");
+ }Signed-off-by: Himanshu Choudhary <Himanshu.Choudhary@amd.com>
| ? cdata.try_add_load_pdi_label(label_arg) | ||
| : cdata.try_add_load_cores_label(label_arg); | ||
| if (!inserted) | ||
| throw error(error::error_code::invalid_asm, |
There was a problem hiding this comment.
warning: statement should be inside braces [hicpp-braces-around-statements]
| throw error(error::error_code::invalid_asm, | |
| rted) { |
src/cpp/preprocessor/asm/asm_parser.cpp:316:
- n");
+ n");
+ }Signed-off-by: Himanshu Choudhary <Himanshu.Choudhary@amd.com>
changed uint32_t to unsigned int |
Problem solved by the commit
https://amd.atlassian.net/wiki/spaces/AIE/pages/1687216605/load_pdi+load_cores+load_cores_cp+preempt+start_cond_job_preempt+check+in+aiebu
load_pdi
load_cores
load_cores_cp
preempt
start_cond_job_preempt
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
Risks (if any) associated the changes in the commit
What has been tested and how, request additional testing if necessary
Documentation impact (if any)