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: turn builders "unpositioned" after emitting a terminator. #84771

Closed
wants to merge 6 commits into from
65 changes: 37 additions & 28 deletions compiler/rustc_codegen_llvm/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,21 +117,36 @@ macro_rules! builder_methods_for_value_instructions {
}
}

// HACK(eddyb) this is an easy way to avoid a complex relationship between
// `Builder` and `UnpositionedBuilder`, even if it seems lopsided.
pub struct UnpositionedBuilder<'a, 'll, 'tcx>(Builder<'a, 'll, 'tcx>);

impl BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
type Unpositioned = UnpositionedBuilder<'a, 'll, 'tcx>;

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

fn position_at_end(bx: Self::Unpositioned, llbb: &'ll BasicBlock) -> Self {
unsafe {
llvm::LLVMPositionBuilderAtEnd(bx.0.llbuilder, llbb);
}
bx.0
}

fn into_unpositioned(self) -> Self::Unpositioned {
UnpositionedBuilder(self)
}

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);
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 }
Self::position_at_end(Self::unpositioned(cx), llbb)
}

fn build_sibling_block(&self, name: &str) -> Self {
Expand All @@ -144,12 +159,6 @@ 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) {
unsafe {
llvm::LLVMPositionBuilderAtEnd(self.llbuilder, llbb);
}
}

fn ret_void(&mut self) {
unsafe {
llvm::LLVMBuildRetVoid(self.llbuilder);
Expand Down Expand Up @@ -384,9 +393,8 @@ impl BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
}

fn alloca(&mut self, ty: &'ll Type, align: Align) -> &'ll Value {
let mut bx = Builder::with_cx(self.cx);
bx.position_at_start(unsafe { llvm::LLVMGetFirstBasicBlock(self.llfn()) });
bx.dynamic_alloca(ty, align)
let entry_llbb = unsafe { llvm::LLVMGetFirstBasicBlock(self.llfn()) };
Self::position_at_start(Self::unpositioned(self.cx), entry_llbb).dynamic_alloca(ty, align)
}

fn dynamic_alloca(&mut self, ty: &'ll Type, align: Align) -> &'ll Value {
Expand Down Expand Up @@ -989,25 +997,25 @@ impl BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
&mut self,
parent: Option<&'ll Value>,
unwind: Option<&'ll BasicBlock>,
num_handlers: usize,
handlers: &[&'ll BasicBlock],
) -> &'ll Value {
let name = cstr!("catchswitch");
let ret = unsafe {
llvm::LLVMRustBuildCatchSwitch(
self.llbuilder,
parent,
unwind,
num_handlers as c_uint,
handlers.len() as c_uint,
name.as_ptr(),
)
};
ret.expect("LLVM does not have support for catchswitch")
}

fn add_handler(&mut self, catch_switch: &'ll Value, handler: &'ll BasicBlock) {
unsafe {
llvm::LLVMRustAddHandler(catch_switch, handler);
let catch_switch = ret.expect("LLVM does not have support for catchswitch");
Copy link
Member

Choose a reason for hiding this comment

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

I believe its been a while since the lowest version of LLVM that we support supports catchswitch (it was introduced in, like, 3.8 or something). This and the Option in the return value can be removed entirely, I think.

for &handler in handlers {
unsafe {
llvm::LLVMRustAddHandler(catch_switch, handler);
}
}
catch_switch
}

fn set_personality_fn(&mut self, personality: &'ll Value) {
Expand Down Expand Up @@ -1169,10 +1177,11 @@ impl Builder<'a, 'll, 'tcx> {
unsafe { llvm::LLVMGetBasicBlockParent(self.llbb()) }
}

fn position_at_start(&mut self, llbb: &'ll BasicBlock) {
fn position_at_start(bx: UnpositionedBuilder<'a, 'll, 'tcx>, llbb: &'ll BasicBlock) -> Self {
unsafe {
llvm::LLVMRustPositionBuilderAtStart(self.llbuilder, llbb);
llvm::LLVMRustPositionBuilderAtStart(bx.0.llbuilder, llbb);
}
bx.0
}

pub fn minnum(&mut self, lhs: &'ll Value, rhs: &'ll Value) -> &'ll Value {
Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_codegen_llvm/src/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -465,9 +465,8 @@ fn codegen_msvc_try(

normal.ret(bx.const_i32(0));

let cs = catchswitch.catch_switch(None, None, 2);
catchswitch.add_handler(cs, catchpad_rust.llbb());
catchswitch.add_handler(cs, catchpad_foreign.llbb());
let cs =
catchswitch.catch_switch(None, None, &[catchpad_rust.llbb(), catchpad_foreign.llbb()]);

// We can't use the TypeDescriptor defined in libpanic_unwind because it
// might be in another DLL and the SEH encoding only supports specifying
Expand Down
4 changes: 1 addition & 3 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1245,9 +1245,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}

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
Bx::position_at_end(Bx::unpositioned(self.cx), self.blocks[bb])
}

fn make_return_dest(
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_codegen_ssa/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,8 +317,7 @@ fn create_funclets<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
let mut cp_bx = bx.build_sibling_block(&format!("cp_funclet{:?}", bb));
ret_llbb = cs_bx.llbb();

let cs = cs_bx.catch_switch(None, None, 1);
cs_bx.add_handler(cs, cp_bx.llbb());
let cs = cs_bx.catch_switch(None, None, &[cp_bx.llbb()]);

// The "null" here is actually a RTTI type descriptor for the
// C++ personality function, but `catch (...)` has no type so
Expand Down
15 changes: 11 additions & 4 deletions compiler/rustc_codegen_ssa/src/traits/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,22 @@ pub trait BuilderMethods<'a, 'tcx>:
+ HasParamEnv<'tcx>
+ HasTargetSpec
{
/// IR builder (like `Self`) that doesn't have a set (insert) position, and
/// cannot be used until positioned (which converted it to `Self`).
// FIXME(eddyb) maybe move this associated type to a different trait, and/or
// provide an `UnpositionedBuilderMethods` trait for operations involving it.
type Unpositioned;

fn unpositioned(cx: &'a Self::CodegenCx) -> Self::Unpositioned;
fn position_at_end(bx: Self::Unpositioned, llbb: Self::BasicBlock) -> Self;
fn into_unpositioned(self) -> Self::Unpositioned;
Copy link
Member

Choose a reason for hiding this comment

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

into_unpositioned is only used inside cg_llvm. Can you remove it from BuilderMethods?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh it's only used there as a shorthand, the main usecase would be hopping between blocks in cg_ssa.
I suppose I could take it out for now, it's trivial to readd later.

Copy link
Member

Choose a reason for hiding this comment

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

Is this hopping between blocks really needed? Can't a block be codegened all at once? That would be necessary once I switch cg_clif to use cg_ssa as cranelift's builder api requires terminating a block before you can switch to the next block.

Copy link
Member Author

Choose a reason for hiding this comment

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

(replied in #84771 (comment) to avoid losing it once I resolve this)


fn new_block<'b>(cx: &'a Self::CodegenCx, llfn: Self::Function, name: &'b str) -> Self;
Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to make this return Self::BasicBlock in a future PR? That should allow using only a single builder for each function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the next step I want to take is to rework how blocks are generated and reuse builders more, but I didn't want to cram too much into one PR at once.

fn with_cx(cx: &'a Self::CodegenCx) -> Self;
fn build_sibling_block(&self, name: &str) -> 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);
fn ret_void(&mut self);
fn ret(&mut self, v: Self::Value);
fn br(&mut self, dest: Self::BasicBlock);
Expand Down Expand Up @@ -245,9 +253,8 @@ pub trait BuilderMethods<'a, 'tcx>:
&mut self,
parent: Option<Self::Value>,
unwind: Option<Self::BasicBlock>,
num_handlers: usize,
handlers: &[Self::BasicBlock],
) -> Self::Value;
fn add_handler(&mut self, catch_switch: Self::Value, handler: Self::BasicBlock);
fn set_personality_fn(&mut self, personality: Self::Value);

fn atomic_cmpxchg(
Expand Down