-
Notifications
You must be signed in to change notification settings - Fork 29
remove preemption scratchpad from .pad #225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main-ge
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,24 +10,6 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| namespace aiebu { | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| void | ||||||||||||||||||||||||||||||||||||||||||||||||||
| aie2ps_encoder:: | ||||||||||||||||||||||||||||||||||||||||||||||||||
| fill_scratchpad(std::shared_ptr<section_writer> padwriter, const std::map<std::string, std::shared_ptr<scratchpad_info>>& scratchpads) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const auto& pad : scratchpads) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const auto& content = pad.second->get_content(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (content.size()) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| assert((void("Pad content size and size doesnt match\n"), content.size() == pad.second->get_size())); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| padwriter->write_bytes(content); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| auto size = pad.second->get_size(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| std::vector<uint8_t> zeros(size, 0x00); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| padwriter->write_bytes(zeros); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| void | ||||||||||||||||||||||||||||||||||||||||||||||||||
| aie2ps_encoder:: | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| aie2ps_encoder:: | |
| aie2ps_encoder:: | |
| fill_scratchpad(std::shared_ptr<section_writer> scratchpadwriter, | |
| const std::vector<char>& scratchpad) | |
| { | |
| scratchpadwriter->write_bytes(scratchpad); | |
| } | |
| void | |
| aie2ps_encoder:: |
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| #include "writer.h" | ||
| #include "aie2ps_preprocessed_output.h" | ||
| #include "ops.h" | ||
| #include "common/logger.h" | ||
| #include "specification/aie2ps/isa.h" | ||
|
|
||
| namespace aiebu { | ||
|
|
@@ -38,6 +39,13 @@ class aie4_encoder : public aie2ps_encoder | |
| uint64_t bd0 = datawriter->read_word(offset); | ||
| uint64_t bd1 = datawriter->read_word(offset + 1*4); // NOLINT | ||
| uint64_t arg = (bd1 & 0xFFFFFFFF) + ((bd0 & 0x1FFFFFF) << 32); // NOLINT | ||
|
|
||
| // 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; | ||
|
Comment on lines
+43
to
+48
|
||
| patch = arg + patch; | ||
| datawriter->write_word_at(offset + 1*4, patch & 0xFFFFFFFF); // NOLINT | ||
| datawriter->write_word_at(offset, (((patch >> 32) & 0x1FFFFFF) | (bd0 & 0xFE000000))); // NOLINT | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,11 +127,11 @@ class aie2ps_preprocessor: public preprocessor | |
| toutput->set_ctrlpkt_id_map(ctrlpkt_id_map); | ||
| toutput->set_annotations(parser->get_annotations()); | ||
|
|
||
| offset_type preemption_scratchpad = 0; | ||
| for (auto col: collist) | ||
|
Comment on lines
+130
to
131
|
||
| { | ||
| std::vector<page> pages; | ||
|
Comment on lines
+130
to
133
|
||
| int relative_page_index = 0; | ||
| int pad_size = 0; | ||
| auto& label_page_index = parser->getcollabelpageindex(col); | ||
| auto& scratchpad = parser->getcolscratchpad(col); | ||
| auto& coldata = parser->get_col_asmdata(col); | ||
|
|
@@ -148,10 +148,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(); | ||
| } | ||
|
Comment on lines
149
to
153
|
||
|
|
||
| toutput->set_coldata(col, pages, scratchpad, label_page_index, tinput->get_control_packet_index()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -196,14 +196,15 @@ class asm_data | |||||||||||||||||||||||||||||
| std::string m_line; | ||||||||||||||||||||||||||||||
| std::string m_file; | ||||||||||||||||||||||||||||||
| int m_annotation_index = -1; | ||||||||||||||||||||||||||||||
| bool m_is_save_restore = false; // True if this instruction is from save/restore routine | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| public: | ||||||||||||||||||||||||||||||
| asm_data() = default; | ||||||||||||||||||||||||||||||
| asm_data(std::shared_ptr<operation> op, operation_type optype, | ||||||||||||||||||||||||||||||
| code_section sec, offset_type size, uint32_t pgnum, | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: 3 adjacent parameters of 'asm_data' of similar type are easily swapped by mistake [bugprone-easily-swappable-parameters] code_section sec, offset_type size, uint32_t pgnum,
^Additional contextsrc/cpp/preprocessor/asm/asm_parser.h:203: the first parameter in the range is 'size' code_section sec, offset_type size, uint32_t pgnum,
^src/cpp/preprocessor/asm/asm_parser.h:204: 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:203: after resolving type aliases, 'offset_type' and 'uint32_t' are the same code_section sec, offset_type 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) | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) {}
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) {}
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: pass by value and use std::move [modernize-pass-by-value] src/cpp/preprocessor/asm/asm_parser.h:206: - 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) {}
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: pass by value and use std::move [modernize-pass-by-value] src/cpp/preprocessor/asm/asm_parser.h:206: - 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) {} |
||||||||||||||||||||||||||||||
| :m_op(op), m_optype(optype), m_section(sec), m_size(size), | ||||||||||||||||||||||||||||||
| m_pagenum(pgnum), m_linenumber(ln), m_line(line), m_file(file) {} | ||||||||||||||||||||||||||||||
| m_pagenum(pgnum), m_linenumber(ln), m_line(line), m_file(file), m_is_save_restore(is_save_restore) {} | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| asm_data( asm_data* a) | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
|
|
@@ -215,6 +216,7 @@ class asm_data | |||||||||||||||||||||||||||||
| a->m_linenumber = m_linenumber; | ||||||||||||||||||||||||||||||
| a->m_line = m_line; | ||||||||||||||||||||||||||||||
| a->m_file = m_file; | ||||||||||||||||||||||||||||||
| a->m_is_save_restore = m_is_save_restore; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| HEADER_ACCESS_GET_SET(code_section, section); | ||||||||||||||||||||||||||||||
|
|
@@ -223,6 +225,7 @@ class asm_data | |||||||||||||||||||||||||||||
| HEADER_ACCESS_GET_SET(uint32_t, linenumber); | ||||||||||||||||||||||||||||||
| HEADER_ACCESS_GET_SET(std::string, file); | ||||||||||||||||||||||||||||||
| HEADER_ACCESS_GET_SET(std::string, line); | ||||||||||||||||||||||||||||||
| HEADER_ACCESS_GET_SET(bool, is_save_restore); | ||||||||||||||||||||||||||||||
| bool isLabel() { return m_optype == operation_type::label; } | ||||||||||||||||||||||||||||||
| bool isOpcode() { return m_optype == operation_type::op; } | ||||||||||||||||||||||||||||||
| bool isAnnotation() { return m_optype == operation_type::annotation; } | ||||||||||||||||||||||||||||||
|
|
@@ -346,6 +349,7 @@ class asm_parser: public std::enable_shared_from_this<asm_parser> | |||||||||||||||||||||||||||||
| std::map<int, std::vector<std::string>> m_preempt_hintmaps; // group -> vector of hintmap_labels (multiple PREEMPT opcodes per group) | ||||||||||||||||||||||||||||||
| std::map<std::string, std::pair<std::string, std::string>> m_hintmap_labels; // hintmap_label -> (save_label, restore_label) | ||||||||||||||||||||||||||||||
| std::set<int> m_preempt_without_hintmap; // groups that have PREEMPT opcodes without hintmaps | ||||||||||||||||||||||||||||||
| bool m_is_save_restore_routine = false; // True when parsing save/restore routine files | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // One unique scratchpad region: all hintmap labels that share the same scratchbase+size | ||||||||||||||||||||||||||||||
| struct hintmap_group_entry { | ||||||||||||||||||||||||||||||
|
|
@@ -425,6 +429,20 @@ class asm_parser: public std::enable_shared_from_this<asm_parser> | |||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| bool is_multi_column_mode() const { return m_preempt_labels.size() > 1; } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Check if currently parsing save/restore routine | ||||||||||||||||||||||||||||||
| bool is_save_restore_routine() const { return m_is_save_restore_routine; } | ||||||||||||||||||||||||||||||
| void set_save_restore_routine(bool val) { m_is_save_restore_routine = val; } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // 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; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
Comment on lines
+436
to
+440
|
||||||||||||||||||||||||||||||
| // 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(); | |
| } |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; | |
| //} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 96b00cbda9f5061f2cfef83b22c2e01d | ||
| 7605802954688a5e7d1fff4198e6ef59 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member variable 'm_is_save_restore_op' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]