remove preemption scratchpad from .pad#225
remove preemption scratchpad from .pad#225HimanshuChoudhary-Xilinx wants to merge 2 commits intoXilinx:main-gefrom
Conversation
Signed-off-by: Himanshu Choudhary <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR updates the assembler/preprocessor/encoder flow to prevent preemption save/restore scratchpads from being emitted into the per-column .pad section, while still supporting correct patching/serialization for save/restore routines.
Changes:
- Add “save/restore routine” tracking through parsing (
asm_data,asm_parser) and propagate it into serialization (assembler_state). - Mark scratchpads as “skip pad section” and update preprocessor/encoder logic to avoid writing those scratchpads into
.pad. - Add additional logging around patching paths and update a test golden MD5.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| test/aie4-ctrlcode/1col_preempt/gold.md5 | Updates expected MD5 to reflect changed scratchpad emission behavior. |
| src/cpp/preprocessor/asm/asm_parser.h | Adds flags/fields to track save/restore routines and “skip pad section” scratchpads. |
| src/cpp/preprocessor/asm/asm_parser.cpp | Propagates save/restore tracking and marks .setpad scratchpads accordingly. |
| src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h | Assigns offsets differently for scratchpads that should not be written to .pad. |
| src/cpp/ops/ops.cpp | Changes how offset_type_marker is rendered for save/restore ops. |
| src/cpp/encoder/aie4/aie4_encoder.h | Adds patching logs in patch57. |
| src/cpp/encoder/aie2ps/aie2ps_encoder.cpp | Skips writing marked scratchpads, adds patching logs, and tracks save/restore ops during serialization. |
| src/cpp/common/assembler_state.h | Adds m_is_save_restore_op to influence serialization behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| offset_type preemption_scratchpad = 0; | ||
| for (auto col: collist) | ||
| { | ||
| std::vector<page> pages; |
There was a problem hiding this comment.
The preemption_scratchpad offsets are not being aligned to 4 bytes (unlike pad_size), which can produce unaligned offsets for skip-pad scratchpads. Consider applying the same 4-byte rounding to preemption_scratchpad before assigning/incrementing, so patching/consumers that assume word alignment don’t break.
| for (auto& pad : scratchpad) | ||
| { | ||
| pad_size = (((pad_size + 3) >> 2) << 2); // round off to next multiple of 4 | ||
| pad.second->set_offset(pad_size); | ||
| pad.second->set_base(PAGE_SIZE * relative_page_index); | ||
| pad_size += pad.second->get_size(); | ||
| if (pad.second->get_skip_pad_section()) { | ||
| pad.second->set_offset(preemption_scratchpad); | ||
| preemption_scratchpad += pad.second->get_size(); | ||
| } else { | ||
| pad_size = (((pad_size + 3) >> 2) << 2); // round off to next multiple of 4 | ||
| pad.second->set_offset(pad_size); | ||
| pad.second->set_base(PAGE_SIZE * relative_page_index); | ||
| pad_size += pad.second->get_size(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The preemption_scratchpad offsets are not being aligned to 4 bytes (unlike pad_size), which can produce unaligned offsets for skip-pad scratchpads. Consider applying the same 4-byte rounding to preemption_scratchpad before assigning/incrementing, so patching/consumers that assume word alignment don’t break.
| offset_type preemption_scratchpad = 0; | ||
| for (auto col: collist) |
There was a problem hiding this comment.
preemption_scratchpad is initialized once before iterating columns, so skip-pad scratchpad offsets will accumulate across columns. If scratchpad offsets are intended to be per-column (matching how pad_size behaves), move/reset preemption_scratchpad inside the per-column loop; otherwise, add an explicit comment explaining why this is intentionally global across all columns.
| // Log patch information using std::cout | ||
| log_info() << "Patching scratchpad: label=" << spad.first | ||
| << ", arg=" << arg | ||
| << ", offset=" << offset | ||
| << ", base=" << page_state->m_scratchpad[spad.first.substr(1)]->get_base() | ||
| << ", offset=" << page_state->m_scratchpad[spad.first.substr(1)]->get_offset() | ||
| << ", patch=" << (page_state->m_scratchpad[spad.first.substr(1)]->get_base() + | ||
| page_state->m_scratchpad[spad.first.substr(1)]->get_offset()) | ||
| << std::endl; | ||
|
|
||
| patch57(textwriter, datawriter, offset + page_header.size(), | ||
| page_state->m_scratchpad[spad.first.substr(1)]->get_base() + page_state->m_scratchpad[spad.first.substr(1)]->get_offset()); |
There was a problem hiding this comment.
This log line is on a potentially hot path and performs multiple operator[] lookups into m_scratchpad (which can also mutate the map if the key is missing). Cache spad.first.substr(1) and the looked-up scratchpad_info once (using find()), and avoid std::endl (it flushes) to reduce overhead.
| // Log patch information using std::cout | |
| log_info() << "Patching scratchpad: label=" << spad.first | |
| << ", arg=" << arg | |
| << ", offset=" << offset | |
| << ", base=" << page_state->m_scratchpad[spad.first.substr(1)]->get_base() | |
| << ", offset=" << page_state->m_scratchpad[spad.first.substr(1)]->get_offset() | |
| << ", patch=" << (page_state->m_scratchpad[spad.first.substr(1)]->get_base() + | |
| page_state->m_scratchpad[spad.first.substr(1)]->get_offset()) | |
| << std::endl; | |
| patch57(textwriter, datawriter, offset + page_header.size(), | |
| page_state->m_scratchpad[spad.first.substr(1)]->get_base() + page_state->m_scratchpad[spad.first.substr(1)]->get_offset()); | |
| // Cache scratchpad key and lookup to avoid repeated map access and substr allocations | |
| const std::string scratchpad_key = spad.first.substr(1); | |
| const auto it_spad = page_state->m_scratchpad.find(scratchpad_key); | |
| if (it_spad == page_state->m_scratchpad.end()) | |
| continue; | |
| const auto &scratchpad = it_spad->second; | |
| // Log patch information | |
| log_info() << "Patching scratchpad: label=" << spad.first | |
| << ", arg=" << arg | |
| << ", offset=" << offset | |
| << ", base=" << scratchpad->get_base() | |
| << ", offset=" << scratchpad->get_offset() | |
| << ", patch=" << (scratchpad->get_base() + scratchpad->get_offset()) | |
| << '\n'; | |
| patch57(textwriter, datawriter, offset + page_header.size(), | |
| scratchpad->get_base() + scratchpad->get_offset()); |
| { | ||
| offset = page_state->parse_num_arg(arg); | ||
|
|
||
| // Log patch information using std::cout |
There was a problem hiding this comment.
The comment says “using std::cout” but the code uses log_info(). Update the comment to match the logger usage (or remove it if it’s redundant).
| // Log patch information using std::cout | |
| // Log patch information |
| log_info() << "aie2ps_encoder::patch57: offset=" << offset | ||
| << ", patch=0x" << std::hex << patch | ||
| << ", arg=0x" << std::hex << arg | ||
| << ", after patch=0x" << std::hex << patch + arg | ||
| << std::dec << std::endl; |
There was a problem hiding this comment.
log_info() in patch57 will execute for every patch operation and can produce very large logs and runtime overhead. Consider lowering to a debug/trace level and/or gating behind a runtime flag to keep normal runs quiet and efficient.
| log_info() << "aie2ps_encoder::patch57: offset=" << offset | |
| << ", patch=0x" << std::hex << patch | |
| << ", arg=0x" << std::hex << arg | |
| << ", after patch=0x" << std::hex << patch + arg | |
| << std::dec << std::endl; | |
| log_debug() << "aie2ps_encoder::patch57: offset=" << offset | |
| << ", patch=0x" << std::hex << patch | |
| << ", arg=0x" << std::hex << arg | |
| << ", after patch=0x" << std::hex << patch + arg | |
| << std::dec << std::endl; |
| // Add log for debugging patching | ||
| log_info() << "aie4_encoder::patch57: offset=" << offset | ||
| << ", patch=0x" << std::hex << patch | ||
| << ", arg=0x" << std::hex << arg | ||
| << ", after patch=0x" << std::hex << patch + arg | ||
| << std::dec << std::endl; |
There was a problem hiding this comment.
Similar to the AIE2PS encoder, this unconditional info-level logging inside a patch routine is likely to be noisy and impact performance. Prefer debug-level logging and/or a feature flag to enable it only when troubleshooting.
| // Check if we should use scratch-pad section for save/restore APPLY_OFFSET_57 | ||
| //bool should_use_scratchpad_section_for_save_restore() const { | ||
| // return m_is_save_restore_routine; | ||
| //} | ||
|
|
There was a problem hiding this comment.
Commented-out code in a public header adds noise and makes it harder to understand the intended API. Either remove this block, or reintroduce it as a real method if it’s still part of the design.
| // Check if we should use scratch-pad section for save/restore APPLY_OFFSET_57 | |
| //bool should_use_scratchpad_section_for_save_restore() const { | |
| // return m_is_save_restore_routine; | |
| //} |
| if (read_pad_file(name, file)) | ||
| if (read_pad_file(name, file, skip_pad_section)) | ||
| return; | ||
| throw error(error::error_code::internal_error, "File " + file + " not exist\n"); |
There was a problem hiding this comment.
The error message is grammatically incorrect and includes a trailing newline. Consider changing it to “does not exist” (and omitting \n) to keep error formatting consistent and easier to consume by callers/logging tools.
| throw error(error::error_code::internal_error, "File " + file + " not exist\n"); | |
| throw error(error::error_code::internal_error, "File " + file + " does not exist"); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| std::vector<std::string> m_labellist; | ||
| std::map<std::string, std::vector<std::string>> m_dependent_labelmap; | ||
| std::set<std::string> m_opt_opcodes; | ||
| bool m_is_save_restore_op = false; // True when currently serializing a save/restore op |
There was a problem hiding this comment.
warning: member variable 'm_is_save_restore_op' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
bool m_is_save_restore_op = false; // True when currently serializing a save/restore op
^| pad_size = (((pad_size + 3) >> 2) << 2); // round off to next multiple of 4 | ||
| pad.second->set_offset(pad_size); | ||
| pad.second->set_base(PAGE_SIZE * relative_page_index); | ||
| pad_size += pad.second->get_size(); |
There was a problem hiding this comment.
warning: narrowing conversion from 'offset_type' (aka 'unsigned int') to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]
pad_size += pad.second->get_size();
^| public: | ||
| asm_data() = default; | ||
| asm_data(std::shared_ptr<operation> op, operation_type optype, | ||
| code_section sec, uint64_t size, uint32_t pgnum, |
There was a problem hiding this comment.
warning: 3 adjacent parameters of 'asm_data' of convertible types are easily swapped by mistake [bugprone-easily-swappable-parameters]
code_section sec, uint64_t size, uint32_t pgnum,
^Additional context
src/cpp/preprocessor/asm/asm_parser.h:199: the first parameter in the range is 'size'
code_section sec, uint64_t size, uint32_t pgnum,
^src/cpp/preprocessor/asm/asm_parser.h:200: the last parameter in the range is 'ln'
uint32_t ln, std::string line, std::string file, bool is_save_restore = false)
^src/cpp/preprocessor/asm/asm_parser.h:199:
code_section sec, uint64_t size, uint32_t pgnum,
^src/cpp/preprocessor/asm/asm_parser.h:199: 'uint64_t' and 'uint32_t' may be implicitly converted: 'uint64_t' (as 'unsigned long') -> 'uint32_t' (as 'unsigned int'), 'uint32_t' (as 'unsigned int') -> 'uint64_t' (as 'unsigned long')
code_section sec, uint64_t size, uint32_t pgnum,
^| asm_data(std::shared_ptr<operation> op, operation_type optype, | ||
| code_section sec, uint64_t size, uint32_t pgnum, | ||
| uint32_t ln, std::string line, std::string file) | ||
| uint32_t ln, std::string line, std::string file, bool is_save_restore = false) |
There was a problem hiding this comment.
warning: pass by value and use std::move [modernize-pass-by-value]
src/cpp/preprocessor/asm/asm_parser.h:202:
- m_pagenum(pgnum), m_linenumber(ln), m_line(line), m_file(file), m_is_save_restore(is_save_restore) {}
+ m_pagenum(pgnum), m_linenumber(ln), m_line(line), m_file(std::move(file)), m_is_save_restore(is_save_restore) {}| asm_data(std::shared_ptr<operation> op, operation_type optype, | ||
| code_section sec, uint64_t size, uint32_t pgnum, | ||
| uint32_t ln, std::string line, std::string file) | ||
| uint32_t ln, std::string line, std::string file, bool is_save_restore = false) |
There was a problem hiding this comment.
warning: pass by value and use std::move [modernize-pass-by-value]
src/cpp/preprocessor/asm/asm_parser.h:202:
- m_pagenum(pgnum), m_linenumber(ln), m_line(line), m_file(file), m_is_save_restore(is_save_restore) {}
+ m_pagenum(pgnum), m_linenumber(ln), m_line(std::move(line)), m_file(file), m_is_save_restore(is_save_restore) {}| public: | ||
| scratchpad_info(std::string name, offset_type size, offset_type offset, offset_type base, std::vector<char>& content): | ||
| m_name(name), m_size(size), m_offset(offset), m_base(base), m_content(std::move(content)) {} | ||
| scratchpad_info(std::string name, offset_type size, offset_type offset, offset_type base, std::vector<char>& content, bool skip_pad_section = false): |
There was a problem hiding this comment.
warning: 3 adjacent parameters of 'scratchpad_info' of similar type ('offset_type') are easily swapped by mistake [bugprone-easily-swappable-parameters]
scratchpad_info(std::string name, offset_type size, offset_type offset, offset_type base, std::vector<char>& content, bool skip_pad_section = false):
^Additional context
src/cpp/preprocessor/asm/asm_parser.h:248: the first parameter in the range is 'size'
scratchpad_info(std::string name, offset_type size, offset_type offset, offset_type base, std::vector<char>& content, bool skip_pad_section = false):
^src/cpp/preprocessor/asm/asm_parser.h:248: the last parameter in the range is 'base'
scratchpad_info(std::string name, offset_type size, offset_type offset, offset_type base, std::vector<char>& content, bool skip_pad_section = false):
^| public: | ||
| scratchpad_info(std::string name, offset_type size, offset_type offset, offset_type base, std::vector<char>& content): | ||
| m_name(name), m_size(size), m_offset(offset), m_base(base), m_content(std::move(content)) {} | ||
| scratchpad_info(std::string name, offset_type size, offset_type offset, offset_type base, std::vector<char>& content, bool skip_pad_section = false): |
There was a problem hiding this comment.
warning: pass by value and use std::move [modernize-pass-by-value]
src/cpp/preprocessor/asm/asm_parser.h:249:
- m_name(name), m_size(size), m_offset(offset), m_base(base), m_content(std::move(content)), m_skip_pad_section(skip_pad_section) {}
+ m_name(std::move(name)), m_size(size), m_offset(offset), m_base(base), m_content(std::move(content)), m_skip_pad_section(skip_pad_section) {}Signed-off-by: Himanshu Choudhary <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if we should skip setpad for this target in save/restore routine | ||
| bool should_skip_setpad_in_save_restore() const { | ||
| return m_is_save_restore_routine; | ||
| } | ||
|
|
There was a problem hiding this comment.
The function name should_skip_setpad_in_save_restore is confusing. It returns true when inside a save/restore routine (line 369), but the comment on line 367 says "Check if we should skip setpad for this target in save/restore routine", which implies it would return true to skip. However, line 493 in the usage comment says ".setpad should only be part of save/restore routines", and the warning is issued when NOT in save/restore (negated condition). Consider renaming to is_in_save_restore_routine() for clarity, or inverting the logic to match the name.
| // Check if we should skip setpad for this target in save/restore routine | |
| bool should_skip_setpad_in_save_restore() const { | |
| return m_is_save_restore_routine; | |
| } | |
| // Returns true if we are in a save/restore routine where .setpad is allowed | |
| bool is_in_save_restore_routine() const { | |
| return m_is_save_restore_routine; | |
| } | |
| // Backward-compatible alias: true means we are in a save/restore routine | |
| // where .setpad is expected/allowed. | |
| bool should_skip_setpad_in_save_restore() const { | |
| return is_in_save_restore_routine(); | |
| } |
| // Log patch information using std::cout | ||
| log_info() << "Patching scratchpad: label=" << spad.first | ||
| << ", arg=" << arg | ||
| << ", offset=" << offset | ||
| << ", base=" << page_state->m_scratchpad[spad.first.substr(1)]->get_base() | ||
| << ", offset=" << page_state->m_scratchpad[spad.first.substr(1)]->get_offset() | ||
| << ", patch=" << (page_state->m_scratchpad[spad.first.substr(1)]->get_base() + | ||
| page_state->m_scratchpad[spad.first.substr(1)]->get_offset()) | ||
| << std::endl; |
There was a problem hiding this comment.
Debug logging statements are being added to production code. These log statements should either be removed before merging or wrapped in a debug flag/conditional compilation to avoid performance impact in production builds.
| // Add log for debugging patching | ||
| log_info() << "aie2ps_encoder::patch57: offset=" << offset | ||
| << ", patch=0x" << std::hex << patch | ||
| << ", arg=0x" << std::hex << arg | ||
| << ", after patch=0x" << std::hex << patch + arg | ||
| << std::dec << std::endl; |
There was a problem hiding this comment.
Debug logging statements are being added to production code. These log statements should either be removed before merging or wrapped in a debug flag/conditional compilation to avoid performance impact in production builds.
| // Add log for debugging patching | |
| log_info() << "aie2ps_encoder::patch57: offset=" << offset | |
| << ", patch=0x" << std::hex << patch | |
| << ", arg=0x" << std::hex << arg | |
| << ", after patch=0x" << std::hex << patch + arg | |
| << std::dec << std::endl; | |
| // Add log for debugging patching | |
| #ifndef NDEBUG | |
| log_info() << "aie2ps_encoder::patch57: offset=" << offset | |
| << ", patch=0x" << std::hex << patch | |
| << ", arg=0x" << std::hex << arg | |
| << ", after patch=0x" << std::hex << patch + arg | |
| << std::dec << std::endl; | |
| #endif |
| } | ||
|
|
||
| void | ||
| aie2ps_encoder:: |
There was a problem hiding this comment.
The fill_scratchpad function implementation has been removed, but its declaration still exists in aie2ps_encoder.h (line 41). This will cause linker errors if the function is ever called. Since the function is no longer used and scratchpad sections are no longer being written (see removed code at lines 55-58 in the old version), the declaration should also be removed from the header file.
| aie2ps_encoder:: | |
| aie2ps_encoder:: | |
| fill_scratchpad(std::shared_ptr<section_writer> scratchpadwriter, | |
| const std::vector<char>& scratchpad) | |
| { | |
| scratchpadwriter->write_bytes(scratchpad); | |
| } | |
| void | |
| aie2ps_encoder:: |
| // for each colnum encode each page | ||
| for (const auto& coldata: totalcoldata) { | ||
| auto colnum = coldata.first; | ||
| for (auto& lpage : coldata.second->m_pages) |
There was a problem hiding this comment.
The variable colnum is extracted from coldata.first but is no longer used after the removal of the scratchpad writing code. This creates an unused variable warning. Consider removing this variable declaration.
| @@ -133,10 +133,8 @@ class aie2ps_preprocessor: public preprocessor | |||
|
|
|||
| for (auto& pad : scratchpad) | |||
| { | |||
| pad_size = (((pad_size + 3) >> 2) << 2); // round off to next multiple of 4 | |||
| pad.second->set_offset(pad_size); | |||
| pad.second->set_base(PAGE_SIZE * relative_page_index); | |||
| pad_size += pad.second->get_size(); | |||
| pad.second->set_offset(preemption_scratchpad); | |||
| preemption_scratchpad += pad.second->get_size(); | |||
| } | |||
There was a problem hiding this comment.
The preemption scratchpad offset calculation has been changed from being per-column with a page-relative base to being global across all columns. This means scratchpad offsets are now accumulated across columns rather than being reset for each column. Verify this behavior is intentional - if multiple columns have scratchpads, they will now have increasing offsets rather than independent offsets. This could be correct if scratchpads are now shared across columns or allocated from a global pool.
Problem solved by the commit
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)
'''
00000ef8 00003706 unrecognized: 6 00000000 53 + 0
00000f20 00003706 unrecognized: 6 00000000 53 + 0
00000f48 00003706 unrecognized: 6 00000000 53 + 0
000012b0 00003806 unrecognized: 6 00000000 55 + 0
00000b00 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000b24 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000b48 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000b6c 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000b90 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000bb4 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000bd8 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000bfc 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000c20 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000c44 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000c68 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000c8c 00003906 unrecognized: 6 00000000 scratch-pad-mem + 0
00000c78 00003a06 unrecognized: 6 00000000 scratch-pad-mem + 0
00000c9c 00003a06 unrecognized: 6 00000000 scratch-pad-mem + 0
00000cc0 00003a06 unrecognized: 6 00000000 scratch-pad-mem + 0
00000ce4 00003a06 unrecognized: 6 00000000 scratch-pad-mem + 0
00000d08 00003a06 unrecognized: 6 00000000 scratch-pad-mem + 0
00000d2c 00003a06 unrecognized: 6 00000000 scratch-pad-mem + 0
Symbol table '.dynsym' contains 59 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 00000000 0 NOTYPE LOCAL DEFAUL
54: 00000000 0 OBJECT GLOBAL DEFAULT 216 52
55: 00000000 0 OBJECT GLOBAL DEFAULT 220 53
56: 00000000 0 OBJECT GLOBAL DEFAULT 220 55
57: 00000000 0 OBJECT GLOBAL DEFAULT 240 scratch-pad-mem
58: 00000000 0 OBJECT GLOBAL DEFAULT 246 scratch-pad-mem