-
Notifications
You must be signed in to change notification settings - Fork 4
Fix some problems and Add more ARM supports #6
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
base: master
Are you sure you want to change the base?
Conversation
Nice work, comments below. |
@@ -23,7 +26,7 @@ enum uarch { | |||
neoverse_v2, | |||
// hisilicon | |||
tsv110, | |||
|
|||
tsv200m, |
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.
Is there any references to this model? If not, I'd better not include this.
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.
You know hisilicon does not release any offical specs, so I'm not very sure. But it refers the micro-architecture on my OrangePi AI Pro.
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.
I understand, but please avoid non-public architecture names.
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.
Could I use the SKU to refer to the architecture? If not set, there might be some problems for users using this hisilicon chip.
endforeach | ||
message('Got CXXFLAGS:', cpp_args) | ||
endif | ||
|
||
if cpu == 'x86_64' |
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.
Don't we already print CXXFLAGS above?
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.
Oh it's for ISA detection.
cpp_args: ['-DAVX2', '-mavx2'], | ||
link_with: utils, | ||
install: true) | ||
if avx2_support |
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.
I think we can just build these binaries without checking if the cpu actually supports it? It allows us to build them on one machine, and run them on another.
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.
It's all right. I'm just afraid that it may cause confuse.
src/elimination.cpp
Outdated
@@ -6,7 +6,7 @@ | |||
|
|||
extern void elimination(FILE *fp); | |||
int main(int argc, char *argv[]) { | |||
FILE *fp = fopen("elimination.csv", "w"); | |||
FILE *fp = fopen("../run_results/elimination.csv", "w"); |
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.
Where does this path come from?
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.
Sorry it's my fault not fixing it before my PR...
@@ -0,0 +1,314 @@ | |||
#include "include/utils.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.
Nice addition, but actually I have done this in my private repo... Let me upstream those code.
src/uarch.cpp
Outdated
} | ||
} | ||
} | ||
fprintf(stderr, "Found CPU family %d, model %d, implementer %d, part %d\n", | ||
family, model, implementer, part); | ||
|
||
if(avx2){ |
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.
Please format code with clang-format
.
src/uarch.cpp
Outdated
@@ -147,6 +187,9 @@ enum uarch get_uarch_inner() { | |||
} else if (implementer == 0x48 && part == 0xd01) { | |||
fprintf(stderr, "Hisilicon TSV110 detected\n"); | |||
return tsv110; | |||
} else if (implementer == 0x00 && part == 0xd02) { |
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.
The 0x00 implementer looks suspicious...
This update expands ARM architecture support by adding NEON and SVE instructions tests, and resolves problems in LLC perf counter configuration and ISA detection. Validation was performed on both x86 (Intel Core i5-8365U/Debian Trixie with AVX2) and ARM platforms (OrangePi AI Pro/Ubuntu 22.04 with SVE), confirming functional correctness across architectures.