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

counter_analysis_toolkit: Automatically detect platform in Makefile #259

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

willowec
Copy link
Contributor

Pull Request Description

When compiling the Counter Analysis Toolkit (CAT) if the wrong architecture is selected for the ARCH variable, relatively confusing compilation errors can occur. For example, compiling on an ARM machine with the default ARCH=X86 setting produces the following GCC error: gcc: error: unrecognized command-line option ‘-mfma’.

To prevent this from happening, make the CAT Makefile use 'uname -m' to detect if the architecture is arm or x86, and otherwise assume powerpc. If ARCH is already set, leave it alone. This change streamlines the workflow of using the CAT on new devices.

Author Checklist

  • Description
    Why this PR exists. Reference all relevant information, including background, issues, test failures, etc
  • Commits
    Commits are self contained and only do one thing
    Commits have a header of the form: module: short description
    Commits have a body (whenever relevant) containing a detailed description of the addressed problem and its solution
  • Tests
    The PR needs to pass all the tests

@Treece-Burgess
Copy link
Contributor

I think this would be a good addition. Personally I think using ifndef ARCH is better than ifeq ($(origin ARCH), undefined). Mainly due to how it is defined in the GNU documentation.

I have tested on an x86 machine and it did work properly. @dbarry9 @adanalis You both have worked much more on this than myself, what do you two think?

@willowec
Copy link
Contributor Author

Thank you for the feedback! ifndef ARCH is definitely more readable as well. I've gone ahead and updated that.

@Treece-Burgess
Copy link
Contributor

@willowec Can you rebase before we merge into master?

…he Makefile

The CAT Makefile will now use 'uname -m' to detect if the
architecture is arm or x86, and otherwise assume powerpc.
If ARCH is already set, it is left alone.
@willowec
Copy link
Contributor Author

Done!

@adanalis adanalis merged commit 4c63af6 into icl-utk-edu:master Oct 24, 2024
26 checks passed
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.

3 participants