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

Fixes https://github.com/sampsyo/bril/issues/342 #343

Merged
merged 5 commits into from
Oct 26, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
25 changes: 19 additions & 6 deletions bril-rs/brillvm/src/llvm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1294,28 +1294,29 @@ pub fn create_module_from_program<'a>(
}
});

(llvm_func, instrs, block, heap)
(llvm_func, instrs, block, heap, return_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

could preserve original function here instead of instrs and return_type
That, or do a zip later with the original functions and the llvm ones

},
)
.collect(); // Important to collect, can't be done lazily because we need all functions to be loaded in before a call instruction of a function is processed.

// Now actually build each function
funcs
.into_iter()
.for_each(|(llvm_func, instrs, mut block, heap)| {
.for_each(|(llvm_func, instrs, mut block, heap, return_type)| {
let mut last_instr = None;

// Maps labels to llvm blocks for jumps
let mut block_map = HashMap::new();

// If their are actually instructions, proceed
if !instrs.is_empty() {
builder.position_at_end(block);

// Maps labels to llvm blocks for jumps
let mut block_map = HashMap::new();
instrs.iter().for_each(|i| match i {
bril_rs::Code::Label { label, .. } => {
let new_block = block_map_get(context, llvm_func, &mut block_map, label);

// Check if wee need to insert a jump since all llvm blocks must be terminated
// Check if we need to insert a jump since all llvm blocks must be terminated
if !is_terminating_instr(&last_instr) {
builder
.build_unconditional_branch(block_map_get(
Expand Down Expand Up @@ -1354,7 +1355,19 @@ pub fn create_module_from_program<'a>(

// Make sure every function is terminated with a return if not already
if !is_terminating_instr(&last_instr) {
builder.build_return(None).unwrap();
if return_type.is_none() {
builder.build_return(None).unwrap();
} else {
// This block did not have a terminating instruction
// Returning void is ill-typed for this function
// This code should be unreachable in well-formed Bril
// Let's just arbitrarily jump to avoid needing to
Copy link
Contributor

Choose a reason for hiding this comment

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

🤣

// instantiate a valid return value.
assert!(!block_map.is_empty());
builder
.build_unconditional_branch(*block_map.values().next().unwrap())
.unwrap();
}
}
});

Expand Down
20 changes: 17 additions & 3 deletions brilift/src/translator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,12 @@ impl CompileEnv<'_> {
}

/// Emit the body of a Bril function into a CLIF function.
fn compile_body(&self, insts: &[bril::Code], builder: &mut FunctionBuilder) {
fn compile_body(
&self,
insts: &[bril::Code],
builder: &mut FunctionBuilder,
return_type: Option<&bril::Type>,
) {
let mut terminated = false; // Entry block is open.
for code in insts {
match code {
Expand Down Expand Up @@ -624,7 +629,16 @@ impl CompileEnv<'_> {

// Implicit return in the last block.
if !terminated {
builder.ins().return_(&[]);
if return_type.is_none() {
builder.ins().return_(&[]);
} else {
// If the function has a return type
// Lets just trap since this must be dead code
builder.ins().trap(
/* Some random trap code */
cranelift_codegen::ir::TrapCode::TableOutOfBounds,
);
}
}
}
}
Expand Down Expand Up @@ -747,7 +761,7 @@ impl<M: Module> Translator<M> {
}

// Insert instructions.
env.compile_body(&func.instrs, &mut builder);
env.compile_body(&func.instrs, &mut builder, func.return_type.as_ref());

builder.seal_all_blocks();
builder.finalize();
Expand Down
10 changes: 10 additions & 0 deletions test/interp/core/dead_block.bril
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
@abs(x: int): int {
ret x;
.label435:
}

@main() {
x : int = const 42;
y : int = call @abs x;
print y;
}
1 change: 1 addition & 0 deletions test/interp/core/dead_block.out
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
42
Loading