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

rustc_codegen_ssa: only create backend BasicBlocks as-needed. #84993

Merged
merged 2 commits into from
May 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
50 changes: 25 additions & 25 deletions compiler/rustc_codegen_llvm/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,24 +118,16 @@ macro_rules! builder_methods_for_value_instructions {
}

impl BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
fn new_block<'b>(cx: &'a CodegenCx<'ll, 'tcx>, llfn: &'ll Value, name: &'b str) -> Self {
let mut bx = Builder::with_cx(cx);
let llbb = unsafe {
let name = SmallCStr::new(name);
llvm::LLVMAppendBasicBlockInContext(cx.llcx, llfn, name.as_ptr())
};
bx.position_at_end(llbb);
fn build(cx: &'a CodegenCx<'ll, 'tcx>, llbb: &'ll BasicBlock) -> Self {
let bx = Builder::with_cx(cx);
unsafe {
llvm::LLVMPositionBuilderAtEnd(bx.llbuilder, llbb);
}
bx
}

fn with_cx(cx: &'a CodegenCx<'ll, 'tcx>) -> Self {
// Create a fresh builder from the crate context.
let llbuilder = unsafe { llvm::LLVMCreateBuilderInContext(cx.llcx) };
Builder { llbuilder, cx }
}

fn build_sibling_block(&self, name: &str) -> Self {
Builder::new_block(self.cx, self.llfn(), name)
fn cx(&self) -> &CodegenCx<'ll, 'tcx> {
self.cx
}

fn llbb(&self) -> &'ll BasicBlock {
Expand All @@ -144,12 +136,22 @@ impl BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {

fn set_span(&mut self, _span: Span) {}

fn position_at_end(&mut self, llbb: &'ll BasicBlock) {
fn append_block(cx: &'a CodegenCx<'ll, 'tcx>, llfn: &'ll Value, name: &str) -> &'ll BasicBlock {
unsafe {
llvm::LLVMPositionBuilderAtEnd(self.llbuilder, llbb);
let name = SmallCStr::new(name);
llvm::LLVMAppendBasicBlockInContext(cx.llcx, llfn, name.as_ptr())
}
}

fn append_sibling_block(&mut self, name: &str) -> &'ll BasicBlock {
Self::append_block(self.cx, self.llfn(), name)
}

fn build_sibling_block(&mut self, name: &str) -> Self {
let llbb = self.append_sibling_block(name);
Self::build(self.cx, llbb)
}

fn ret_void(&mut self) {
unsafe {
llvm::LLVMBuildRetVoid(self.llbuilder);
Expand Down Expand Up @@ -1144,14 +1146,6 @@ impl BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
unsafe { llvm::LLVMBuildZExt(self.llbuilder, val, dest_ty, UNNAMED) }
}

fn cx(&self) -> &CodegenCx<'ll, 'tcx> {
self.cx
}

unsafe fn delete_basic_block(&mut self, bb: &'ll BasicBlock) {
llvm::LLVMDeleteBasicBlock(bb);
}

fn do_not_inline(&mut self, llret: &'ll Value) {
llvm::Attribute::NoInline.apply_callsite(llvm::AttributePlace::Function, llret);
}
Expand All @@ -1165,6 +1159,12 @@ impl StaticBuilderMethods for Builder<'a, 'll, 'tcx> {
}

impl Builder<'a, 'll, 'tcx> {
fn with_cx(cx: &'a CodegenCx<'ll, 'tcx>) -> Self {
// Create a fresh builder from the crate context.
let llbuilder = unsafe { llvm::LLVMCreateBuilderInContext(cx.llcx) };
Builder { llbuilder, cx }
}

pub fn llfn(&self) -> &'ll Value {
unsafe { llvm::LLVMGetBasicBlockParent(self.llbb()) }
}
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,8 @@ fn declare_unused_fn(cx: &CodegenCx<'ll, 'tcx>, def_id: &DefId) -> Instance<'tcx

fn codegen_unused_fn_and_counter(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<'tcx>) {
let llfn = cx.get_fn(instance);
let mut bx = Builder::new_block(cx, llfn, "unused_function");
let llbb = Builder::append_block(cx, llfn, "unused_function");
let mut bx = Builder::build(cx, llbb);
let fn_name = bx.get_pgo_func_name_var(instance);
let hash = bx.const_u64(0);
let num_counters = bx.const_u32(1);
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_codegen_llvm/src/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,8 @@ fn gen_fn<'ll, 'tcx>(
cx.apply_target_cpu_attr(llfn);
// FIXME(eddyb) find a nicer way to do this.
unsafe { llvm::LLVMRustSetLinkage(llfn, llvm::Linkage::InternalLinkage) };
let bx = Builder::new_block(cx, llfn, "entry-block");
let llbb = Builder::append_block(cx, llfn, "entry-block");
let bx = Builder::build(cx, llbb);
codegen(bx);
llfn
}
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_codegen_llvm/src/llvm/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1079,7 +1079,6 @@ extern "C" {
Fn: &'a Value,
Name: *const c_char,
) -> &'a BasicBlock;
pub fn LLVMDeleteBasicBlock(BB: &BasicBlock);

// Operations on instructions
pub fn LLVMIsAInstruction(Val: &Value) -> Option<&Value>;
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_codegen_ssa/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,8 @@ pub fn maybe_create_entry_wrapper<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
cx.set_frame_pointer_elimination(llfn);
cx.apply_target_cpu_attr(llfn);

let mut bx = Bx::new_block(&cx, llfn, "top");
let llbb = Bx::append_block(&cx, llfn, "top");
let mut bx = Bx::build(&cx, llbb);

bx.insert_reference_to_gdb_debug_scripts_section_global();

Expand Down
39 changes: 27 additions & 12 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
target: mir::BasicBlock,
) -> (Bx::BasicBlock, bool) {
let span = self.terminator.source_info.span;
let lltarget = fx.blocks[target];
let lltarget = fx.llbb(target);
let target_funclet = fx.cleanup_kinds[target].funclet_bb(target);
match (self.funclet_bb, target_funclet) {
(None, None) => (lltarget, false),
Expand Down Expand Up @@ -133,13 +133,13 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
// If there is a cleanup block and the function we're calling can unwind, then
// do an invoke, otherwise do a call.
if let Some(cleanup) = cleanup.filter(|_| fn_abi.can_unwind) {
let ret_bx = if let Some((_, target)) = destination {
fx.blocks[target]
let ret_llbb = if let Some((_, target)) = destination {
fx.llbb(target)
} else {
fx.unreachable_block()
};
let invokeret =
bx.invoke(fn_ptr, &llargs, ret_bx, self.llblock(fx, cleanup), self.funclet(fx));
bx.invoke(fn_ptr, &llargs, ret_llbb, self.llblock(fx, cleanup), self.funclet(fx));
bx.apply_attrs_callsite(&fn_abi, invokeret);

if let Some((ret_dest, target)) = destination {
Expand Down Expand Up @@ -386,7 +386,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

// Create the failure block and the conditional branch to it.
let lltarget = helper.llblock(self, target);
let panic_block = self.new_block("panic");
let panic_block = bx.build_sibling_block("panic");
if expected {
bx.cond_br(cond, lltarget, panic_block.llbb());
} else {
Expand Down Expand Up @@ -1205,7 +1205,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

// FIXME(eddyb) rename this to `eh_pad_for_uncached`.
fn landing_pad_for_uncached(&mut self, bb: mir::BasicBlock) -> Bx::BasicBlock {
let llbb = self.blocks[bb];
let llbb = self.llbb(bb);
if base::wants_msvc_seh(self.cx.sess()) {
let funclet;
let ret_llbb;
Expand Down Expand Up @@ -1289,14 +1289,29 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
})
}

pub fn new_block(&self, name: &str) -> Bx {
Bx::new_block(self.cx, self.llfn, name)
// FIXME(eddyb) replace with `build_sibling_block`/`append_sibling_block`
// (which requires having a `Bx` already, and not all callers do).
fn new_block(&self, name: &str) -> Bx {
let llbb = Bx::append_block(self.cx, self.llfn, name);
Bx::build(self.cx, llbb)
}

pub fn build_block(&self, bb: mir::BasicBlock) -> Bx {
let mut bx = Bx::with_cx(self.cx);
bx.position_at_end(self.blocks[bb]);
bx
/// Get the backend `BasicBlock` for a MIR `BasicBlock`, either already
/// cached in `self.cached_llbbs`, or created on demand (and cached).
// FIXME(eddyb) rename `llbb` and other `ll`-prefixed things to use a
// more backend-agnostic prefix such as `cg` (i.e. this would be `cgbb`).
pub fn llbb(&mut self, bb: mir::BasicBlock) -> Bx::BasicBlock {
self.cached_llbbs[bb].unwrap_or_else(|| {
// FIXME(eddyb) only name the block if `fewer_names` is `false`.
Copy link
Member

Choose a reason for hiding this comment

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

Would it not make sense to just check for this within append_block function? I guess if we did that, we'd end up in a situation where we still format! potentially many strings only for them to be ignored?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - I think I want to solve it similar to how I did for SSA values and set_name on them, I just don't want to do it now.

let llbb = Bx::append_block(self.cx, self.llfn, &format!("{:?}", bb));
self.cached_llbbs[bb] = Some(llbb);
llbb
})
}

pub fn build_block(&mut self, bb: mir::BasicBlock) -> Bx {
let llbb = self.llbb(bb);
Bx::build(self.cx, llbb)
}

fn make_return_dest(
Expand Down
55 changes: 22 additions & 33 deletions compiler/rustc_codegen_ssa/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,11 @@ pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> {
/// then later loaded when generating the DIVERGE_BLOCK.
personality_slot: Option<PlaceRef<'tcx, Bx::Value>>,

/// A `Block` for each MIR `BasicBlock`
blocks: IndexVec<mir::BasicBlock, Bx::BasicBlock>,
/// A backend `BasicBlock` for each MIR `BasicBlock`, created lazily
/// as-needed (e.g. RPO reaching it or another block branching to it).
// FIXME(eddyb) rename `llbbs` and other `ll`-prefixed things to use a
// more backend-agnostic prefix such as `cg` (i.e. this would be `cgbbs`).
cached_llbbs: IndexVec<mir::BasicBlock, Option<Bx::BasicBlock>>,

/// The funclet status of each basic block
cleanup_kinds: IndexVec<mir::BasicBlock, analyze::CleanupKind>,
Expand Down Expand Up @@ -141,7 +144,8 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(

let debug_context = cx.create_function_debug_context(instance, &fn_abi, llfn, &mir);

let mut bx = Bx::new_block(cx, llfn, "start");
let start_llbb = Bx::append_block(cx, llfn, "start");
let mut bx = Bx::build(cx, start_llbb);

if mir.basic_blocks().iter().any(|bb| bb.is_cleanup) {
bx.set_personality_fn(cx.eh_personality());
Expand All @@ -151,17 +155,17 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
// Allocate a `Block` for every basic block, except
// the start block, if nothing loops back to it.
let reentrant_start_block = !mir.predecessors()[mir::START_BLOCK].is_empty();
let block_bxs: IndexVec<mir::BasicBlock, Bx::BasicBlock> = mir
.basic_blocks()
.indices()
.map(|bb| {
if bb == mir::START_BLOCK && !reentrant_start_block {
bx.llbb()
} else {
bx.build_sibling_block(&format!("{:?}", bb)).llbb()
}
})
.collect();
let cached_llbbs: IndexVec<mir::BasicBlock, Option<Bx::BasicBlock>> =
mir.basic_blocks()
.indices()
.map(|bb| {
if bb == mir::START_BLOCK && !reentrant_start_block {
Some(start_llbb)
} else {
None
}
})
.collect();

let mut fx = FunctionCx {
instance,
Expand All @@ -170,7 +174,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
fn_abi,
cx,
personality_slot: None,
blocks: block_bxs,
cached_llbbs,
unreachable_block: None,
cleanup_kinds,
landing_pads: IndexVec::from_elem(None, mir.basic_blocks()),
Expand Down Expand Up @@ -245,29 +249,14 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(

// Branch to the START block, if it's not the entry block.
if reentrant_start_block {
bx.br(fx.blocks[mir::START_BLOCK]);
bx.br(fx.llbb(mir::START_BLOCK));
}

let rpo = traversal::reverse_postorder(&mir);
let mut visited = BitSet::new_empty(mir.basic_blocks().len());

// Codegen the body of each block using reverse postorder
for (bb, _) in rpo {
visited.insert(bb.index());
// FIXME(eddyb) reuse RPO iterator between `analysis` and this.
for (bb, _) in traversal::reverse_postorder(&mir) {
fx.codegen_block(bb);
}

// Remove blocks that haven't been visited, or have no
// predecessors.
for bb in mir.basic_blocks().indices() {
// Unreachable block
if !visited.contains(bb.index()) {
debug!("codegen_mir: block {:?} was not visited", bb);
unsafe {
bx.delete_basic_block(fx.blocks[bb]);
}
}
}
}

/// Produces, for each argument, a `Value` pointing at the
Expand Down
16 changes: 11 additions & 5 deletions compiler/rustc_codegen_ssa/src/traits/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,21 @@ pub trait BuilderMethods<'a, 'tcx>:
+ HasParamEnv<'tcx>
+ HasTargetSpec
{
fn new_block<'b>(cx: &'a Self::CodegenCx, llfn: Self::Function, name: &'b str) -> Self;
fn with_cx(cx: &'a Self::CodegenCx) -> Self;
fn build_sibling_block(&self, name: &str) -> Self;
fn build(cx: &'a Self::CodegenCx, llbb: Self::BasicBlock) -> Self;

fn cx(&self) -> &Self::CodegenCx;
fn llbb(&self) -> Self::BasicBlock;

fn set_span(&mut self, span: Span);

fn position_at_end(&mut self, llbb: Self::BasicBlock);
// FIXME(eddyb) replace uses of this with `append_sibling_block`.
fn append_block(cx: &'a Self::CodegenCx, llfn: Self::Function, name: &str) -> Self::BasicBlock;

fn append_sibling_block(&mut self, name: &str) -> Self::BasicBlock;

// FIXME(eddyb) replace with callers using `append_sibling_block`.
fn build_sibling_block(&mut self, name: &str) -> Self;

fn ret_void(&mut self);
fn ret(&mut self, v: Self::Value);
fn br(&mut self, dest: Self::BasicBlock);
Expand Down Expand Up @@ -291,6 +298,5 @@ pub trait BuilderMethods<'a, 'tcx>:
) -> Self::Value;
fn zext(&mut self, val: Self::Value, dest_ty: Self::Type) -> Self::Value;

unsafe fn delete_basic_block(&mut self, bb: Self::BasicBlock);
fn do_not_inline(&mut self, llret: Self::Value);
}
9 changes: 6 additions & 3 deletions src/test/codegen/drop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,16 @@ pub fn droppy() {
// FIXME(eddyb) the `void @` forces a match on the instruction, instead of the
// comment, that's `; call core::ptr::drop_in_place::<drop::SomeUniqueName>`
// for the `v0` mangling, should switch to matching on that once `legacy` is gone.
// CHECK-NOT: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK-NOT: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK-NOT: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK-NOT: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK-NOT: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK-NOT: invoke void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK: call void @{{.*}}drop_in_place{{.*}}SomeUniqueName
// CHECK-NOT: {{(call|invoke) void @.*}}drop_in_place{{.*}}SomeUniqueName
// The next line checks for the } that ends the function definition
Expand Down
6 changes: 3 additions & 3 deletions src/test/codegen/match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ pub fn exhaustive_match(e: E) -> u8 {
// CHECK-NEXT: i[[TY:[0-9]+]] [[DISCR:[0-9]+]], label %[[A:[a-zA-Z0-9_]+]]
// CHECK-NEXT: i[[TY:[0-9]+]] [[DISCR:[0-9]+]], label %[[B:[a-zA-Z0-9_]+]]
// CHECK-NEXT: ]
// CHECK: [[B]]:
// CHECK-NEXT: store i8 1, i8* %1, align 1
// CHECK-NEXT: br label %[[EXIT:[a-zA-Z0-9_]+]]
// CHECK: [[OTHERWISE]]:
// CHECK-NEXT: unreachable
// CHECK: [[A]]:
// CHECK-NEXT: store i8 0, i8* %1, align 1
// CHECK-NEXT: br label %[[EXIT:[a-zA-Z0-9_]+]]
// CHECK: [[B]]:
Copy link
Member

Choose a reason for hiding this comment

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

This suggests to me these checks want to be CHECK-DAG but sounds like it'd be a major PiTA to adjust.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a CHECK-DAG feature? I might look into it, it's just these two tests.

// CHECK-NEXT: store i8 1, i8* %1, align 1
// CHECK-NEXT: br label %[[EXIT:[a-zA-Z0-9_]+]]
match e {
E::A => 0,
Expand Down