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.

Sign up for GitHub

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

Jump to bottom

Added default target cpu to --print target-cpus output and updated docs #110876

Merged
merged 4 commits into from
May 5, 2023
Merged

Added default target cpu to --print target-cpus output and updated docs #110876

merged 4 commits into from
May 5, 2023

Conversation

mj10021
Copy link
Contributor

@mj10021 mj10021 commented Apr 27, 2023

Added default target cpu info as requested in issue #110647 and noted the new output in the documentation

@rustbot
Copy link
Collaborator

rustbot commented Apr 27, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 27, 2023
@compiler-errors
Copy link
Member

Can you show how this actually renders, so the reviewer doesn't need to test it themself?

@mj10021
Copy link
Contributor Author

mj10021 commented Apr 27, 2023

Can you show how this actually renders, so the reviewer doesn't need to test it themself?

Sure thing. Running rustc --print target-cpus shows:

Default cpu for this target:
    x86-64
Available CPUs for this target:
    native         - Select the CPU of the current host (currently skylake).
    alderlake     
    amdfam10      
    athlon
   ...

and running rustc --print target-cpus --target i686-unknown-linux-gnu shows:

Default cpu for this target:
    pentium4
Available CPUs for this target:
    alderlake     
    amdfam10      
    athlon 
   ...

@mj10021
Copy link
Contributor Author

mj10021 commented Apr 27, 2023

Can you show how this actually renders, so the reviewer doesn't need to test it themself?

Sure thing. Running rustc --print target-cpus shows:

Default cpu for this target:
    x86-64
Available CPUs for this target:
    native         - Select the CPU of the current host (currently skylake).
    alderlake     
    amdfam10      
    athlon
   ...

and running rustc --print target-cpus --target i686-unknown-linux-gnu shows:

Default cpu for this target:
    pentium4
Available CPUs for this target:
    alderlake     
    amdfam10      
    athlon 
   ...

Changing cpu to CPU to match the previous text

@compiler-errors
Copy link
Member

I think the comment #110647 (comment) was asking for this to be rendered inline, as a note after the default CPU flavor. I agree with that comment. How hard is it to change the behavior to act like that instead?

@mj10021
Copy link
Contributor Author

mj10021 commented Apr 27, 2023

I think the comment #110647 (comment) was asking for this to be rendered inline, as a note after the default CPU flavor. I agree with that comment. How hard is it to change the behavior to act like that instead?

Got it, working on this now, I think I just need to pass the default target cpu into unsafe { llvm::LLVMRustPrintTargetCPUs(tm) }

@mj10021
Copy link
Contributor Author

mj10021 commented Apr 28, 2023

This is the new output:

rustc --print target-cpus 

Available CPUs for this target:
    native         - Select the CPU of the current host (currently skylake).
    alderlake
    amdfam10
    athlon
    athlon-4
    athlon-fx
    athlon-mp
    athlon-tbird
    athlon-xp
    athlon64
    athlon64-sse3
    atom
    barcelona
    bdver1
    bdver2
    bdver3
    bdver4
    bonnell
    broadwell
    btver1
    btver2
    c3
    c3-2
    cannonlake
    cascadelake
    cooperlake
    core-avx-i
    core-avx2
    core2
    corei7
    corei7-avx
    emeraldrapids
    generic
    geode
    goldmont
    goldmont-plus
    grandridge
    graniterapids
    haswell
    i386
    i486
    i586
    i686
    icelake-client
    icelake-server
    ivybridge
    k6
    k6-2
    k6-3
    k8
    k8-sse3
    knl
    knm
    lakemont
    meteorlake
    nehalem
    nocona
    opteron
    opteron-sse3
    penryn
    pentium
    pentium-m
    pentium-mmx
    pentium2
    pentium3
    pentium3m
    pentium4
    pentium4m
    pentiumpro
    prescott
    raptorlake
    rocketlake
    sandybridge
    sapphirerapids
    sierraforest
    silvermont
    skx
    skylake
    skylake-avx512
    slm
    tigerlake
    tremont
    westmere
    winchip-c6
    winchip2
    x86-64           - this is the default target cpu
    x86-64-v2
    x86-64-v3
    x86-64-v4
    yonah
    znver1
    znver2
    znver3
    znver4

Let me know if I did something wrong with the C++, I don't have much experience there. Thanks!

@mj10021
Copy link
Contributor Author

mj10021 commented Apr 28, 2023

Maybe the text should read - this is the default target cpu for the current build target ?

@mj10021
Copy link
Contributor Author

mj10021 commented Apr 28, 2023

Maybe the text should read - this is the default target cpu for the current build target ?

also I need to capitalize CPU again lol

@mj10021
Copy link
Contributor Author

mj10021 commented Apr 30, 2023 •
edited
Loading

Updated the output to
x86-64 - This is the default target CPU for the current build target (currently x86_64-unknown-linux-gnu).
and changed .expect() to .unwrap_or_else()

b-naber
b-naber reviewed May 4, 2023
View reviewed changes
compiler/rustc_codegen_llvm/src/llvm_util.rs
PrintRequest::TargetCPUs => {
let cpu_cstring = CString::new(handle_native(sess.target.cpu.as_ref()))
.unwrap_or_else(|e| bug!("failed to convert to cstring: {}", e));
unsafe { llvm::LLVMRustPrintTargetCPUs(tm, cpu_cstring.as_ptr()) };
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Maybe a SAFETY comment here explaining why this is ok (had to look at the docs for CString to check that it is)?

@mj10021 mj10021 force-pushed the issue-110647-fix branch from 4f76695 to cb74cd5 Compare May 5, 2023 00:54
@b-naber
Copy link
Contributor

b-naber commented May 5, 2023

Thanks. @bors r+ rollup

@bors
Copy link
Contributor

bors commented May 5, 2023

📌 Commit f239cd6 has been approved by b-naber

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2023
@Dylan-DPC Dylan-DPC mentioned this pull request May 5, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 5, 2023
@bors
Auto merge of rust-lang#111248 - Dylan-DPC:rollup-lbp0ui3, r=Dylan-DPC
4b94c23 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#103056 (Fix `checked_{add,sub}_duration` incorrectly returning `None` when `other` has more than `i64::MAX` seconds)
 - rust-lang#108801 (Implement RFC 3348, `c"foo"` literals)
 - rust-lang#110773 (Reduce MIR dump file count for MIR-opt tests)
 - rust-lang#110876 (Added default target cpu to `--print target-cpus` output and updated docs)
 - rust-lang#111068 (Improve check-cfg implementation)
 - rust-lang#111238 (btree_map: `Cursor{,Mut}::peek_prev` must agree)

Failed merges:

 - rust-lang#110694 (Implement builtin # syntax and use it for offset_of!(...))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 65702bf into rust-lang:master May 5, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 5, 2023
@workingjubilee workingjubilee mentioned this pull request Jul 22, 2023
Closed
@mj10021 mj10021 deleted the issue-110647-fix branch November 1, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b-naber b-naber b-naber left review comments

Assignees

@b-naber b-naber

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.71.0
Development

Successfully merging this pull request may close these issues.
5 participants
@mj10021 @rustbot @compiler-errors @b-naber @bors