-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Correct some codegen stats counter inconsistencies #52171
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
Conversation
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
| } | ||
|
|
||
| pub fn add_incoming_to_phi(&self, phi: ValueRef, val: ValueRef, bb: BasicBlockRef) { | ||
| self.count_insn("addincoming"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already an addincoming
rust/src/librustc_codegen_llvm/builder.rs
Lines 831 to 834 in c6807bb
| pub fn phi(&self, ty: Type, vals: &[ValueRef], bbs: &[BasicBlockRef]) -> ValueRef { | |
| assert_eq!(vals.len(), bbs.len()); | |
| let phi = self.empty_phi(ty); | |
| self.count_insn("addincoming"); |
I wonder wether the we want to differentiate between these two. Otherwise, r=me. CC @alexcrichton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't dig too far into it, but it looked like the phi gave you an object you can call add_to_phi on to add more items to. Both the phi and add_to_phi generate 1 AddIncoming instruction. Additionally, the phi call calls empty_phi which generates a BuildPhi instruction and increments the emptyphi instruction.
I added the count because it is generating that AddIncoming but only the initial phi was incrementing the counter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah AFAIK these are all debugging counters anyway, so this should be fine!
|
@bors r+ |
|
📌 Commit aac0d91 has been approved by |
|
@bors rollup |
Correct some codegen stats counter inconsistencies I noticed some possible typos/inconsistencies in the codegen counters. For example, `fsub` was getting counted as an integer `sub`, whereas `fadd` was counted as an add. And `addincoming` was only being counted on the initial call. https://github.com/rust-lang/rust/blob/dbd10f81758381339f98994b8d31814cf5e98707/src/librustc_codegen_llvm/builder.rs#L831-L841 Only remaining inconsistencies I can see are things like `fadd_fast` are counted as `fadd`. But the vector versions like `vector_reduce_fmax_fast` are counted as `vector.reduce.fmax_fast` not as their 'base' versions (`vector_reduce_fmax` is counted as `vector.reduce.fmax`).
Correct some codegen stats counter inconsistencies I noticed some possible typos/inconsistencies in the codegen counters. For example, `fsub` was getting counted as an integer `sub`, whereas `fadd` was counted as an add. And `addincoming` was only being counted on the initial call. https://github.com/rust-lang/rust/blob/dbd10f81758381339f98994b8d31814cf5e98707/src/librustc_codegen_llvm/builder.rs#L831-L841 Only remaining inconsistencies I can see are things like `fadd_fast` are counted as `fadd`. But the vector versions like `vector_reduce_fmax_fast` are counted as `vector.reduce.fmax_fast` not as their 'base' versions (`vector_reduce_fmax` is counted as `vector.reduce.fmax`).
Rollup of 7 pull requests Successful merges: - #51612 (NLL: fix E0594 "change to mutable ref" suggestion) - #51722 (Updated RELEASES for 1.28.0) - #52064 (Clarifying how the alignment of the struct works) - #52149 (Add #[repr(transparent)] to Atomic* types) - #52151 (Trait impl settings) - #52171 (Correct some codegen stats counter inconsistencies) - #52195 (rustc: Avoid /tmp/ in graphviz writing) Failed merges: - #52164 (use proper footnote syntax for references) r? @ghost
I noticed some possible typos/inconsistencies in the codegen counters. For example,
fsubwas getting counted as an integersub, whereasfaddwas counted as an add. Andaddincomingwas only being counted on the initial call.rust/src/librustc_codegen_llvm/builder.rs
Lines 831 to 841 in dbd10f8
Only remaining inconsistencies I can see are things like
fadd_fastare counted asfadd. But the vector versions likevector_reduce_fmax_fastare counted asvector.reduce.fmax_fastnot as their 'base' versions (vector_reduce_fmaxis counted asvector.reduce.fmax).