Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## latest #3293 +/- ##
==========================================
+ Coverage 73.31% 73.56% +0.24%
==========================================
Files 446 446
Lines 108397 109543 +1146
Branches 17360 17547 +187
==========================================
+ Hits 79476 80588 +1112
- Misses 28645 28678 +33
- Partials 276 277 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Quick question: are you wanting to use openblas_get_num_threads for anything else?
An alternate implementation would be to modify the set_num_threads to also return the number of threads:
int openblas_set_num_threads(int num_threads) {
#if defined(HIPO_USES_OPENBLAS)
openblas_set_num_threads(num_threads);
return openblas_get_num_threads();
#else
return num_threads;
#endif
}This would make most of the other changes unnecessary.
|
|
mathgeekcoder
left a comment
There was a problem hiding this comment.
Looking better! I've added a couple more optional suggestions
|
|
||
| static bool usingAppleBlas() { | ||
| return strstr(HighsExtras::blas::getInfo()->provider, "Apple") != nullptr; | ||
| std::string provider = HighsExtras::blas::getInfo()->provider; |
There was a problem hiding this comment.
Perhaps we can include something like this in stringutil.h?
#if defined(_WIN32) || defined(_WIN64)
#include <string.h> // _stricmp
#else
#include <strings.h> // strcasecmp
#endif
inline int stringcasecmp(const char* lhs, const char* rhs) {
#if defined(_WIN32) || defined(_WIN64)
return _stricmp(lhs, rhs);
#else
return strcasecmp(lhs, rhs);
#endif
}Then this becomes:
static bool usingAppleBlas() {
return stringcasecmp(HighsExtras::blas::getInfo()->provider, "apple") == 0;
}It should be faster and cleaner than creating a string, converting to lower and comparing.
There was a problem hiding this comment.
Alternatively, we could modify HighsExtrasFeatureInfo:
bool HighsExtrasFeatureInfo::isProvider(const char* check) {
return stringcasecmp(provider, check) == 0;
}such that:
HighsExtras::blas::getInfo()->isProvider("apple");There was a problem hiding this comment.
Yes, this looks neat, I will make the changes
There was a problem hiding this comment.
We need to check for substrings, not exact match though (so that "Apple Accelerate" matches "apple"). I replaced strcasecmp with strcasestr. Unfortunately there seems to be no equivalent on Windows. One possible solution is to use _strnicmp iteratively. I am not sure of whether this is fast, but it shouldn't be a big deal. The providers are short and need to be checked only a couple of times.
There was a problem hiding this comment.
I'm having some issues to include stringutil.h in HighsExtrasApiBinding.h in a way that works everywhere, in order to implement isProvider. I will remove isProvider for now.
| } | ||
|
|
||
| static bool usingOpenBLAS() { | ||
| std::string provider = HighsExtras::blas::getInfo()->provider; |
There was a problem hiding this comment.
As above
static bool usingOpenBLAS() {
return stringcasecmp(HighsExtras::blas::getInfo()->provider, "openblas") == 0;
}| function(highs_configure_blas_metadata) | ||
| set(HIGHS_BLAS_COMPILE_DEFINITION "" PARENT_SCOPE) | ||
|
|
||
| string(TOLOWER "${BLAS_LIBRARIES}" BLAS_LIBRARIES_LOWER) |
There was a problem hiding this comment.
Sorry, can you explain why this is needed?
There was a problem hiding this comment.
If I use OpenBLAS by passing libopenblas.so to BLAS_LIBRARIES, CMake doesn't know which vendor is being used and so it doesn't set the BLAS metadata (i.e., the provider is unknown, etc). Crucially, this means that HIPO_USES_OPENBLAS is not set, and OpenBLAS runs multi-threaded, with a substantial reduction in performance.
Maybe there are better ways of achieving this.
Description
openblas_get_num_threads, to query the number of threads used by OpenBLAS.HIPO_USES_OPENBLASis passed to HiPO.BLAS_LIBRARIESis used and the provided library contains the stringopenblas, set the BLAS metadata so that HiPO knows that OpenBLAS is being used.Checklist
latestbranch