Skip to content

[XDP] Updates to AIE Debug plugin across various devices#8817

Closed
pgschuey wants to merge 14 commits intoXilinx:masterfrom
pgschuey:aieDebugPluginV2
Closed

[XDP] Updates to AIE Debug plugin across various devices#8817
pgschuey wants to merge 14 commits intoXilinx:masterfrom
pgschuey:aieDebugPluginV2

Conversation

@pgschuey
Copy link
Collaborator

Problem solved by the commit

  • Only 32 bit registers were supported
  • Common register locations across tile types were getting mislabeled
  • Some files were too large and were slowing down compilation

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

  • Added support for larger register sizes
  • Redefined and classified registers by tile/module types
  • Separated out generational info into separate files

Risks (if any) associated the changes in the commit

  • Lots of device types and generations supported

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

  • Tested on Edge, client, and VE2

Documentation impact (if any)

  • N/A

Snigdha Gupta and others added 8 commits February 11, 2025 15:55
Signed-off-by: Snigdha Gupta <snigupta@xsjsnigupta40x.xlnx.xilinx.com>
Signed-off-by: Prerona Dutta <predutta@xcopredutta50x.xlnx.xilinx.com>
Signed-off-by: Prerona Dutta <predutta@xcopredutta50x.xlnx.xilinx.com>
Signed-off-by: Prerona Dutta <predutta@xcopredutta50x.xlnx.xilinx.com>
Signed-off-by: Prerona Dutta <predutta@xcopredutta50x.xlnx.xilinx.com>
Signed-off-by: Paul Schumacher <schuey@xilinx.com>
Signed-off-by: Paul Schumacher <schuey@xilinx.com>
@pgschuey pgschuey marked this pull request as draft March 14, 2025 15:28
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 70. Check the log or trigger a new build to see more.

#include "xdp/profile/plugin/vp_base/vp_base_plugin.h"
#include "xdp/profile/plugin/aie_debug/used_registers.h"

extern "C" {
#include <xaiengine.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'xaiengine.h' file not found [clang-diagnostic-error]

#include <xaiengine.h>
         ^


extern "C" {
#include <xaiengine.h>
#include <xaiengine/xaiegbl_params.h>
}

#define NUMBEROFMODULES 4
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: macro 'NUMBEROFMODULES' used to declare a constant; consider using a 'constexpr' constant [cppcoreguidelines-macro-usage]

#define NUMBEROFMODULES 4
        ^

}

void printValues(uint32_t deviceID, VPDatabase* db) {
int i = 0;
std::vector<uint64_t>* addrVectors[] = {&coreRelativeOffsets, &memoryRelativeOffsets, &shimRelativeOffsets, &memTileRelativeOffsets};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

      std::vector<uint64_t>* addrVectors[] = {&coreRelativeOffsets, &memoryRelativeOffsets, &shimRelativeOffsets, &memTileRelativeOffsets};
                           ^

}

void printValues(uint32_t deviceID, VPDatabase* db) {
int i = 0;
std::vector<uint64_t>* addrVectors[] = {&coreRelativeOffsets, &memoryRelativeOffsets, &shimRelativeOffsets, &memTileRelativeOffsets};
std::vector<xdp::aie::AieDebugValue>* valueVectors[] = {&coreValues, &memoryValues, &shimValues, &memTileValues};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

      std::vector<xdp::aie::AieDebugValue>* valueVectors[] = {&coreValues, &memoryValues, &shimValues, &memTileValues};
                                          ^

…x.xilinx.com>\n I, Snigdha Gupta <snigupta@xsjsnigupta40x.xlnx.xilinx.com>, hereby add my Signed-off-by to this commit: d396431\n Signed-off-by: Snigdha Gupta <snigupta@xsjsnigupta40x.xlnx.xilinx.com>

Signed-off-by: Snigdha Gupta <snigupta@xsjsnigupta50x.xlnx.xilinx.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 45. Check the log or trigger a new build to see more.

/*************************************************************************************
AIE1 Registers
*************************************************************************************/
class AIE1UsedRegisters : public UsedRegisters {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

class AIE1UsedRegisters : public UsedRegisters {
      ^

class AIE1UsedRegisters : public UsedRegisters {
public:
AIE1UsedRegisters() {
populateRegNameToValueMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: Call to virtual method 'AIE1UsedRegisters::populateRegNameToValueMap' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

    populateRegNameToValueMap();
    ^
Additional context

src/runtime_src/xdp/profile/plugin/aie_debug/used_registers.h:172: Call to virtual method 'AIE1UsedRegisters::populateRegNameToValueMap' during construction bypasses virtual dispatch

    populateRegNameToValueMap();
    ^

}
virtual ~AIE2UsedRegisters() {}

void populateProfileRegisters();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]

Suggested change
void populateProfileRegisters();
void populateProfileRegisters() override;

virtual ~AIE2UsedRegisters() {}

void populateProfileRegisters();
void populateTraceRegisters();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]

Suggested change
void populateTraceRegisters();
void populateTraceRegisters() override;


void populateProfileRegisters();
void populateTraceRegisters();
void populateRegNameToValueMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]

Suggested change
void populateRegNameToValueMap();
void populateRegNameToValueMap() override;

void populateProfileRegisters();
void populateTraceRegisters();
void populateRegNameToValueMap();
void populateRegValueToNameMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]

Suggested change
void populateRegValueToNameMap();
void populateRegValueToNameMap() override;

void populateTraceRegisters();
void populateRegNameToValueMap();
void populateRegValueToNameMap();
void populateRegAddrToSizeMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]

Suggested change
void populateRegAddrToSizeMap();
void populateRegAddrToSizeMap() override;

…x.xilinx.com>

I, Snigdha Gupta <snigupta@xsjsnigupta40x.xlnx.xilinx.com>, hereby add my Signed-off-by to this commit: d396431

Signed-off-by: Snigdha Gupta <snigupta@xsjsnigupta40x.xlnx.xilinx.com>
Signed-off-by: Snigdha Gupta <snigupta@xsjsnigupta50x.xlnx.xilinx.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

/*************************************************************************************
AIE2ps Registers
*************************************************************************************/
class AIE2psUsedRegisters : public UsedRegisters {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

class AIE2psUsedRegisters : public UsedRegisters {
      ^

class AIE2psUsedRegisters : public UsedRegisters {
public:
AIE2psUsedRegisters() {
populateRegNameToValueMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: Call to virtual method 'AIE2psUsedRegisters::populateRegNameToValueMap' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

    populateRegNameToValueMap();
    ^
Additional context

src/runtime_src/xdp/profile/plugin/aie_debug/used_registers.h:210: Call to virtual method 'AIE2psUsedRegisters::populateRegNameToValueMap' during construction bypasses virtual dispatch

    populateRegNameToValueMap();
    ^

public:
AIE2psUsedRegisters() {
populateRegNameToValueMap();
populateRegValueToNameMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: Call to virtual method 'AIE2psUsedRegisters::populateRegValueToNameMap' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

    populateRegValueToNameMap();
    ^
Additional context

src/runtime_src/xdp/profile/plugin/aie_debug/used_registers.h:211: Call to virtual method 'AIE2psUsedRegisters::populateRegValueToNameMap' during construction bypasses virtual dispatch

    populateRegValueToNameMap();
    ^

AIE2psUsedRegisters() {
populateRegNameToValueMap();
populateRegValueToNameMap();
populateRegAddrToSizeMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: Call to virtual method 'AIE2psUsedRegisters::populateRegAddrToSizeMap' during construction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

    populateRegAddrToSizeMap();
    ^
Additional context

src/runtime_src/xdp/profile/plugin/aie_debug/used_registers.h:212: Call to virtual method 'AIE2psUsedRegisters::populateRegAddrToSizeMap' during construction bypasses virtual dispatch

    populateRegAddrToSizeMap();
    ^

populateRegValueToNameMap();
populateRegAddrToSizeMap();
}
virtual ~AIE2psUsedRegisters() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [hicpp-use-override]

Suggested change
virtual ~AIE2psUsedRegisters() {}
~AIE2psUsedRegisters() override {}

RegisterInterpreter(uint64_t deviceIndex, int aieGeneration);

~RegisterInterpreter()=default;
RegisterInterpreter(int aieGeneration);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [hicpp-explicit-conversions]

Suggested change
RegisterInterpreter(int aieGeneration);
explicit RegisterInterpreter(int aieGeneration);


~RegisterInterpreter()=default;
RegisterInterpreter(int aieGeneration);
~RegisterInterpreter() {};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use '= default' to define a trivial destructor [hicpp-use-equals-default]

Suggested change
~RegisterInterpreter() {};
~RegisterInterpreter() = default;

std::vector<uint32_t> subval;

RegInfo(std::string f, std::string b, uint32_t s)
RegInfo(std::string f, std::string b, std::vector<uint32_t> s)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: constructor does not initialize these fields: subval [cppcoreguidelines-pro-type-member-init]

src/runtime_src/xdp/profile/writer/aie_debug/register_interpreter.h:36:

-       std::vector<uint32_t> subval;
+       std::vector<uint32_t> subval{};

std::vector<uint32_t> subval;

RegInfo(std::string f, std::string b, uint32_t s)
RegInfo(std::string f, std::string b, std::vector<uint32_t> s)
Copy link
Contributor

Choose a reason for hiding this comment

The 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/runtime_src/xdp/profile/writer/aie_debug/register_interpreter.h:23:

- #include <vector>
+ #include <utility>
+ #include <vector>

src/runtime_src/xdp/profile/writer/aie_debug/register_interpreter.h:39:

-         : field_name(f), bit_range(b), subval(s) {}
+         : field_name(std::move(f)), bit_range(b), subval(s) {}

std::vector<uint32_t> subval;

RegInfo(std::string f, std::string b, uint32_t s)
RegInfo(std::string f, std::string b, std::vector<uint32_t> s)
Copy link
Contributor

Choose a reason for hiding this comment

The 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/runtime_src/xdp/profile/writer/aie_debug/register_interpreter.h:39:

-         : field_name(f), bit_range(b), subval(s) {}
+         : field_name(f), bit_range(std::move(b)), subval(s) {}

Paul Schumacher and others added 4 commits March 21, 2025 11:02
…x.xilinx.com>

I, Snigdha Gupta <snigupta@xsjsnigupta40x.xlnx.xilinx.com>, hereby add my Signed-off-by to this commit: d396431

Signed-off-by: Snigdha Gupta <snigupta@xsjsnigupta40x.xlnx.xilinx.com>
Signed-off-by: Snigdha Gupta <snigupta@xsjsnigupta50x.xlnx.xilinx.com>
Signed-off-by: Prerona Dutta <predutta@xcopredutta50x.xlnx.xilinx.com>
@pgschuey pgschuey closed this Apr 21, 2025
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.

2 participants

Comments