aie4 changes by HimanshuChoudhary-Xilinx · Pull Request #86 · Xilinx/aiebu

HimanshuChoudhary-Xilinx · 2025-05-02T17:35:40Z

Problem solved by the commit

Aiebu change to support aie4 target

added new target asm_aie4
aie2ps and aie4 have 2 things different
a. patch57 schema and control packet schema
b. actor_id's for tiles/mm2s/s2mm
created assembler_state base class and aie2ps and aie4 derived class. We have create_assembler_state virtual function which will return aie4 or aie2ps assembler_state shared ptr.
created aie4 encoder class from aie2ps encoder as only patch57 and assember_state is different.

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

NA

How problem was solved, alternative solutions (if any) and why they were rejected

NA

Risks (if any) associated the changes in the commit

What has been tested and how, request additional testing if necessary

Documentation impact (if any)

USAGE

> aiebu-asm -t aie2ps -c ..\test\cpp_test\aie2ps\eff_net_coal\ml_asm\merged_control.asm -L ..\test\cpp_test\aie2ps\eff_net_coal\ml_asm -L ..\test\cpp_test\aie2ps\eff_net_coal\asm\ -o controlcode.elf

github-actions

clang-tidy made some suggestions

github-actions · 2025-05-02T17:41:56Z

 };

-class assembler_state
+class assembler_state : public std::enable_shared_from_this<assembler_state>


warning: class 'assembler_state' defines a default destructor, a copy constructor, a copy assignment operator and a move constructor but does not define a move assignment operator [cppcoreguidelines-special-member-functions]

class assembler_state : public std::enable_shared_from_this<assembler_state> ^

github-actions · 2025-05-02T17:41:56Z

+                  std::map<std::string, std::shared_ptr<scratchpad_info>>& scratchpad,
+                  std::map<std::string, uint32_t>& labelpageindex, uint32_t control_packet_index, bool makeunique);
+  assembler_state(const assembler_state& rhs) = default;
+  assembler_state& operator=(const assembler_state& rhs) = default;


warning: deleted member function should be public [hicpp-use-equals-delete]

assembler_state& operator=(const assembler_state& rhs) = default; ^

github-actions · 2025-05-02T17:41:56Z

+                  std::map<std::string, std::shared_ptr<scratchpad_info>>& scratchpad,
+                  std::map<std::string, uint32_t>& labelpageindex, uint32_t control_packet_index, bool makeunique);
+  assembler_state(const assembler_state& rhs) = default;
+  assembler_state& operator=(const assembler_state& rhs) = default;


warning: explicitly defaulted copy assignment operator is implicitly deleted [clang-diagnostic-defaulted-function-deleted]

assembler_state& operator=(const assembler_state& rhs) = default; ^

Additional context

src/cpp/common/assembler_state.h:149: copy assignment operator of 'assembler_state' is implicitly deleted because field 'm_data' is of reference type 'std::vector<std::shared_ptr<asm_data>> &'

std::vector<std::shared_ptr<asm_data>>& m_data; ^

src/cpp/common/assembler_state.h:145: replace 'default' with 'delete'

assembler_state& operator=(const assembler_state& rhs) = default; ^

github-actions · 2025-05-02T17:41:56Z

    COALESED_BUFFER
  };

+  symbol::patch_schema control_packet_patching = symbol::patch_schema::control_packet_57;


warning: member variable 'control_packet_patching' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

symbol::patch_schema control_packet_patching = symbol::patch_schema::control_packet_57; ^

stsoe

There are other fundamental changes, such as pointer semantics. Please describe the changes in the PR summary, rather than just a one liner.

xuhz · 2025-05-02T18:33:31Z

+  get_actor_id_map() const override
+  {
+    static std::unordered_map<std::string, ActionId> actor_id = {
+      {"mm2s", {5, 6}},               //NOLINT


aie4, if tile type is not specified, it is an error.
so line 246-247 should be removed

xuhz · 2025-05-02T18:38:16Z

+      {"s2mm", {5, 0}},               //NOLINT
+      {"tile_mm2s", {10, 6}},         //NOLINT
+      {"tile_s2mm", {10, 0}},         //NOLINT
+      {"shim_mm2s", {10, 6}},         //NOLINT


please see table 3-23 in latest spec
this part now is very ugly ...
memtile, the mm2s actor id has a hole
shimtile, actor id of s2mm 0 & 1 are not contiguous
there is also a trace_s2mm channel

check to see whether the data structure define can handle all these case.

checked and fixed

stsoe

LGTM

Signed-off-by: HimanshuChoudhary-Xilinx <Himanshu.Choudhary@amd.com>

github-actions

clang-tidy made some suggestions

github-actions · 2025-05-08T11:23:52Z

+                  std::map<std::string, std::shared_ptr<scratchpad_info>>& scratchpad,
+                  std::map<std::string, uint32_t>& labelpageindex, uint32_t control_packet_index, bool makeunique);
+  assembler_state(const assembler_state& rhs) = default;
+  assembler_state& operator=(const assembler_state& rhs) = default;


warning: explicitly defaulted copy assignment operator is implicitly deleted [clang-diagnostic-defaulted-function-deleted]

assembler_state& operator=(const assembler_state& rhs) = default; ^

Additional context

src/cpp/common/assembler_state.h:148: copy assignment operator of 'assembler_state' is implicitly deleted because field 'm_data' is of reference type 'std::vector<std::shared_ptr<asm_data>> &'

std::vector<std::shared_ptr<asm_data>>& m_data; ^

src/cpp/common/assembler_state.h:144: replace 'default' with 'delete'

assembler_state& operator=(const assembler_state& rhs) = default; ^

HimanshuChoudhary-Xilinx requested review from chvamshi-xilinx, sonals and stsoe May 2, 2025 17:36

github-actions Bot reviewed May 2, 2025

View reviewed changes

stsoe reviewed May 2, 2025

View reviewed changes

xuhz requested changes May 2, 2025

View reviewed changes

stsoe approved these changes May 6, 2025

View reviewed changes

chvamshi-xilinx approved these changes May 7, 2025

View reviewed changes

HimanshuChoudhary-Xilinx added 2 commits May 8, 2025 16:46

aie4 changes

1cfa874

Signed-off-by: HimanshuChoudhary-Xilinx <Himanshu.Choudhary@amd.com>

fix actor id's

68861bf

Signed-off-by: HimanshuChoudhary-Xilinx <Himanshu.Choudhary@amd.com>

HimanshuChoudhary-Xilinx force-pushed the aie4_changes_1 branch from 69d7854 to 68861bf Compare May 8, 2025 11:17

github-actions Bot reviewed May 8, 2025

View reviewed changes

xuhz approved these changes May 9, 2025

View reviewed changes

chvamshi-xilinx merged commit ad258eb into Xilinx:main-ge May 9, 2025
4 checks passed

Conversation

HimanshuChoudhary-Xilinx commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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)

USAGE

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 2, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 2, 2025

Choose a reason for hiding this comment

Uh oh!

stsoe left a comment

Choose a reason for hiding this comment

Uh oh!

xuhz May 2, 2025

Choose a reason for hiding this comment

Uh oh!

HimanshuChoudhary-Xilinx May 8, 2025

Choose a reason for hiding this comment

Uh oh!

xuhz May 2, 2025

Choose a reason for hiding this comment

Uh oh!

HimanshuChoudhary-Xilinx May 8, 2025

Choose a reason for hiding this comment

Uh oh!

stsoe left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot May 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HimanshuChoudhary-Xilinx commented May 2, 2025 •

edited

Loading