-
Notifications
You must be signed in to change notification settings - Fork 487
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
base: master
Are you sure you want to change the base?
Conversation
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>
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.
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> |
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: 'xaiengine.h' file not found [clang-diagnostic-error]
#include <xaiengine.h>
^
|
||
extern "C" { | ||
#include <xaiengine.h> | ||
#include <xaiengine/xaiegbl_params.h> | ||
} | ||
|
||
#define NUMBEROFMODULES 4 |
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: 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}; |
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: 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}; |
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: 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>
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.
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 { |
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: 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(); |
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: 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(); |
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: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]
void populateProfileRegisters(); | |
void populateProfileRegisters() override; |
virtual ~AIE2UsedRegisters() {} | ||
|
||
void populateProfileRegisters(); | ||
void populateTraceRegisters(); |
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: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]
void populateTraceRegisters(); | |
void populateTraceRegisters() override; |
|
||
void populateProfileRegisters(); | ||
void populateTraceRegisters(); | ||
void populateRegNameToValueMap(); |
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: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]
void populateRegNameToValueMap(); | |
void populateRegNameToValueMap() override; |
void populateProfileRegisters(); | ||
void populateTraceRegisters(); | ||
void populateRegNameToValueMap(); | ||
void populateRegValueToNameMap(); |
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: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]
void populateRegValueToNameMap(); | |
void populateRegValueToNameMap() override; |
void populateTraceRegisters(); | ||
void populateRegNameToValueMap(); | ||
void populateRegValueToNameMap(); | ||
void populateRegAddrToSizeMap(); |
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: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]
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>
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.
clang-tidy made some suggestions
/************************************************************************************* | ||
AIE2ps Registers | ||
*************************************************************************************/ | ||
class AIE2psUsedRegisters : public UsedRegisters { |
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: 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(); |
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: 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(); |
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: 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(); |
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: 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() {} |
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: prefer using 'override' or (rarely) 'final' instead of 'virtual' [hicpp-use-override]
virtual ~AIE2psUsedRegisters() {} | |
~AIE2psUsedRegisters() override {} |
RegisterInterpreter(uint64_t deviceIndex, int aieGeneration); | ||
|
||
~RegisterInterpreter()=default; | ||
RegisterInterpreter(int aieGeneration); |
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: single-argument constructors must be marked explicit to avoid unintentional implicit conversions [hicpp-explicit-conversions]
RegisterInterpreter(int aieGeneration); | |
explicit RegisterInterpreter(int aieGeneration); |
|
||
~RegisterInterpreter()=default; | ||
RegisterInterpreter(int aieGeneration); | ||
~RegisterInterpreter() {}; |
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: use '= default' to define a trivial destructor [hicpp-use-equals-default]
~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) |
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: 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) |
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: 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) |
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: 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) {}
…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>
Problem solved by the commit
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)