Skip to content

maintain order of insertion in controlcode leabels#261

Merged
sonals merged 2 commits intoXilinx:main-gefrom
HimanshuChoudhary-Xilinx:labelorder
Apr 13, 2026
Merged

maintain order of insertion in controlcode leabels#261
sonals merged 2 commits intoXilinx:main-gefrom
HimanshuChoudhary-Xilinx:labelorder

Conversation

@HimanshuChoudhary-Xilinx
Copy link
Copy Markdown
Collaborator

@HimanshuChoudhary-Xilinx HimanshuChoudhary-Xilinx commented Apr 13, 2026

Problem solved by the commit

asm_parser::getLabelsforcol() previously built its list by iterating col_data’s std::map<std::string, section_asmdata>, which orders keys lexicographically, not in the order labels first appear in the assembly.
The aie2ps preprocessor walks labels in that order when pagifying and can place controlcode in any order in in elf.

This pr return getlabelsforcol() in order they were inserted, keeping main controlcode at page0

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)

Signed-off-by: Himanshu Choudhary <Himanshu.Choudhary@amd.com>
Copilot AI review requested due to automatic review settings April 13, 2026 05:17
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to ensure control-code label processing preserves the original parse/insertion order (instead of relying on std::map key ordering), so downstream paging/assembly happens in a deterministic, source-defined sequence.

Changes:

  • Added explicit label insertion-order tracking in col_data and exposed it via get_label_insertion_order().
  • Updated label creation in asm_parser::insert_col_asmdata() to funnel through ensure_label(), and changed getLabelsforcol() to return insertion order.
  • Added additional debug logging while processing labels/pages in the AIE2PS preprocessor.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/cpp/preprocessor/asm/asm_parser.h Adds label insertion-order vector + API changes to return labels in insertion order.
src/cpp/preprocessor/asm/asm_parser.cpp Ensures label initialization uses the new insertion-order mechanism during ASM data insertion.
src/cpp/preprocessor/aie2ps/aie2ps_preprocessor.h Consumes the new label ordering and adds debug logs for label/page processing.
Comments suppressed due to low confidence (2)

src/cpp/preprocessor/asm/asm_parser.h:382

  • get_label_asmdata() uses m_label_data[label], which will default-insert a new entry if the label doesn’t exist. With the new m_label_insertion_order bookkeeping, that insertion bypasses ensure_label(), leaving the internal state inconsistent (label exists in the map but never appears in getLabelsforcol()). Consider using find()/at() and throwing (or returning empty) instead of mutating the map in a getter, and ensure all label creation routes through ensure_label().
  std::vector<std::shared_ptr<asm_data>> get_label_asmdata(const std::string& label)
  {
    std::vector<std::shared_ptr<asm_data>> result(m_label_data[label].text);
    result.insert(result.end(), m_label_data[label].data.begin(), m_label_data[label].data.end());
    return result;

src/cpp/preprocessor/asm/asm_parser.cpp:67

  • After introducing ensure_label() to track insertion order, it’s important that subsequent accesses don’t accidentally create labels via operator[]. Even though ensure_label(m_current_label) is called here, using label_data[m_current_label] still relies on that ordering invariant and will silently create a new label if m_current_label is ever not ensured (e.g., future refactors/callers). Prefer auto& section = label_data.at(m_current_label); (after ensure_label) to enforce the invariant and avoid hidden mutations.
  auto& cdata = m_col[m_current_col];
  cdata.ensure_label(m_current_label);
  auto& label_data = cdata.get_label_data();

  if (get_data_state())
    label_data[m_current_label].data.emplace_back(data);
  else
    label_data[m_current_label].text.emplace_back(data);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


class col_data
{
// First time a label scope receives asm_data; map iteration order is order of insertion.
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says “map iteration order is order of insertion”, but std::map iterates in key-sorted order, not insertion order. This is misleading now that insertion order is actually tracked in m_label_insertion_order; update the comment to reflect that the vector preserves insertion order while the map remains sorted by key.

Suggested change
// First time a label scope receives asm_data; map iteration order is order of insertion.
// Tracks the first time a label scope receives asm_data, preserving insertion order.
// Label data itself is stored in m_label_data, whose std::map iteration remains key-sorted.

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Himanshu Choudhary <Himanshu.Choudhary@amd.com>
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@sonals sonals self-requested a review April 13, 2026 15:54
@sonals sonals merged commit 861a91b into Xilinx:main-ge Apr 13, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants