Skip to content

Correct generation of GETRF files by the CMAKE build #2420

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

Merged
merged 1 commit into from
Feb 15, 2020
Merged

Correct generation of GETRF files by the CMAKE build #2420

merged 1 commit into from
Feb 15, 2020

Conversation

martin-frbg
Copy link
Collaborator

fixes #2396

@martin-frbg martin-frbg added this to the 0.3.9 milestone Feb 15, 2020
@martin-frbg martin-frbg merged commit 785c389 into OpenMathLib:develop Feb 15, 2020
@marxin
Copy link
Contributor

marxin commented Feb 15, 2020

You found it, good job! It seems to me a more common problem and I have one suggestion. Instead of:

#define MAXPS	maxps
#define MAXSS	maxss
#ifdef USE_MIN
#define MAXPS	minps
#define MAXSS	minss
#endif

I would rather use:

#if defined(USE_MAX)
#define MAXPS	maxps
#define MAXSS	maxss
#elif defined(USE_MIN)
#define MAXPS	minps
#define MAXSS	minss
#else
#error Neither USE_MAX nor USE_MIN is defined
#endif

It will take some time to change it but it may pay off in the future?

@martin-frbg
Copy link
Collaborator Author

Err, that comment belongs to the iamax_sse.S of #2414 or what ? I do not see a relevance to the -DUNIT problem here (where the #define plays a role in the included common.h) and neither will anybody who may try to understand code changes through reading the associated git history a few months from now.

@marxin
Copy link
Contributor

marxin commented Feb 16, 2020

No, it's related to this issue as well. It's about a missing macro definition. In the case of USE_MIN, if you forget to define it somewhere you end up with a wrong code. With my suggestion, you will end up with a compilation error. That seems to me much nicer than a silent miscompilation.

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.

dgesv produces incorrect answer when compiling the library using CMake with USE_OPENMP=1
2 participants