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

Store Used CpuFeature in Artifact instead of Present CpuFeatures for Singlepass #3363

Merged
merged 13 commits into from
Nov 30, 2022

Conversation

ptitSeb
Copy link
Contributor

@ptitSeb ptitSeb commented Nov 23, 2022

Description

Added a function in Compiler trait to list the potentially used CpuFeatures. This is then used when storing the Artifact.
Singlepass compiler only use a handfull of cpufeature on x86_64, so it's usefull there to avoid marking artifact as needing features like AVX512 while it's not used at all.

For #3362

@ptitSeb ptitSeb requested a review from syrusakbary as a code owner November 23, 2022 10:30
@ptitSeb ptitSeb changed the title Store Used CpuFeature in Artifact instead Present CpuFeatures for Singlepass Store Used CpuFeature in Artifact instead of Present CpuFeatures for Singlepass Nov 23, 2022
Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't run it, but given the original issue's description, this PR looks fine to me.

How hard would this sort of thing be to test?

lib/compiler-singlepass/src/compiler.rs Show resolved Hide resolved
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for this?

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Nov 24, 2022

I added some unit test for the get_cpu_features_used function

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Nov 28, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 28, 2022

👎 Rejected by code reviews

@syrusakbary
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Nov 30, 2022
3363: Store Used CpuFeature in Artifact instead of Present CpuFeatures for Singlepass r=syrusakbary a=ptitSeb

# Description
Added a function in Compiler trait to list the potentially used CpuFeatures. This is then used when storing the Artifact.
Singlepass compiler only use a handfull of cpufeature on x86_64, so it's usefull there to avoid marking artifact as needing features like AVX512 while it's not used at all.

For #3362 

Co-authored-by: ptitSeb <[email protected]>
@syrusakbary syrusakbary merged commit df8307e into master Nov 30, 2022
@syrusakbary syrusakbary deleted the feat_store-used-cpufeature-artifact branch November 30, 2022 21:06
@bors
Copy link
Contributor

bors bot commented Nov 30, 2022

Build failed:

  • Code lint

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