Skip to content
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

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

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

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

@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>

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

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};

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};

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

@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 {

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();

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();

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();

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();

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();

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();

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

@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 {

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();

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();

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();

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() {}

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);

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() {};

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;


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

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{};


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

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) {}


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

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>
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