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

Correct some codegen stats counter inconsistencies #52171

Merged
merged 1 commit into from
Jul 11, 2018
Merged

Correct some codegen stats counter inconsistencies #52171

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/librustc_codegen_llvm/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}

pub fn nswsub(&self, lhs: ValueRef, rhs: ValueRef) -> ValueRef {
self.count_insn("nwsub");
self.count_insn("nswsub");
unsafe {
llvm::LLVMBuildNSWSub(self.llbuilder, lhs, rhs, noname())
}
Expand All @@ -291,14 +291,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}

pub fn fsub(&self, lhs: ValueRef, rhs: ValueRef) -> ValueRef {
self.count_insn("sub");
self.count_insn("fsub");
unsafe {
llvm::LLVMBuildFSub(self.llbuilder, lhs, rhs, noname())
}
}

pub fn fsub_fast(&self, lhs: ValueRef, rhs: ValueRef) -> ValueRef {
self.count_insn("sub");
self.count_insn("fsub");
unsafe {
let instr = llvm::LLVMBuildFSub(self.llbuilder, lhs, rhs, noname());
llvm::LLVMRustSetHasUnsafeAlgebra(instr);
Expand Down Expand Up @@ -1315,6 +1315,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}

pub fn add_incoming_to_phi(&self, phi: ValueRef, val: ValueRef, bb: BasicBlockRef) {
self.count_insn("addincoming");
Copy link
Contributor

@estebank estebank Jul 9, 2018 •
edited
Loading

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

Copy link
Contributor Author

@bharrisau bharrisau Jul 9, 2018 •
edited
Loading

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.

Copy link
Member

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!

unsafe {
llvm::LLVMAddIncoming(phi, &val, &bb, 1 as c_uint);
}
Expand Down