Skip to content

Commit

Permalink
[Cider] Improve conflicting assignment error message (#2282)
Browse files Browse the repository at this point in the history 
Small tweak for cider which modifies the way errors are processed and
improves the error message for conflicting assignments to print both
assignments and their origin.

Also adds the data converter flags to the `fud2` flow.
  • Loading branch information
@EclecticGriffin
EclecticGriffin authored Sep 16, 2024
1 parent a1a2b3e commit ce58e29
Show file tree
Hide file tree
Showing 14 changed files with 323 additions and 71 deletions.
9 changes: 6 additions & 3 deletions fud2/scripts/cider.rhai
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,16 @@ fn cider_setup(e) {
);
e.arg("pool", "console");

e.config_var_or("converter-flags", "cider.converter-flags", "");
e.config_var_or("cider-flags", "cider.flags", "");

e.rule(
"run-cider",
"$cider-exe -l $calyx-base --data data.dump $in > $out",
"$cider-exe -l $calyx-base --data data.dump $cider-flags $in > $out",
);

e.rule("dump-to-interp", "$cider-converter --to cider $in > $out");
e.rule("interp-to-dump", "$cider-converter --to json $in > $out");
e.rule("dump-to-interp", "$cider-converter --to cider $converter-flags $in > $out");
e.rule("interp-to-dump", "$cider-converter --to json $converter-flags $in > $out");
e.build_cmd(
["data.dump"],
"dump-to-interp",
Expand Down
8 changes: 5 additions & 3 deletions fud2/tests/snapshots/tests__test@calyx_through_cider_to_dat.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@ cider-converter = $calyx-base/target/debug/cider-data-converter
rule run-cider-debug
command = $cider-exe -l $calyx-base --data data.dump $in debug || true
pool = console
converter-flags =
cider-flags =
rule run-cider
command = $cider-exe -l $calyx-base --data data.dump $in > $out
command = $cider-exe -l $calyx-base --data data.dump $cider-flags $in > $out
rule dump-to-interp
command = $cider-converter --to cider $in > $out
command = $cider-converter --to cider $converter-flags $in > $out
rule interp-to-dump
command = $cider-converter --to json $in > $out
command = $cider-converter --to json $converter-flags $in > $out
build data.dump: dump-to-interp $sim_data | $cider-converter

build pseudo_cider: calyx-with-flags _from_stdin_calyx.futil
Expand Down
8 changes: 5 additions & 3 deletions fud2/tests/snapshots/tests__test@calyx_to_cider-debug.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@ cider-converter = $calyx-base/target/debug/cider-data-converter
rule run-cider-debug
command = $cider-exe -l $calyx-base --data data.dump $in debug || true
pool = console
converter-flags =
cider-flags =
rule run-cider
command = $cider-exe -l $calyx-base --data data.dump $in > $out
command = $cider-exe -l $calyx-base --data data.dump $cider-flags $in > $out
rule dump-to-interp
command = $cider-converter --to cider $in > $out
command = $cider-converter --to cider $converter-flags $in > $out
rule interp-to-dump
command = $cider-converter --to json $in > $out
command = $cider-converter --to json $converter-flags $in > $out
build data.dump: dump-to-interp $sim_data | $cider-converter

build pseudo_cider: calyx-with-flags _from_stdin_calyx.futil
Expand Down
8 changes: 5 additions & 3 deletions fud2/tests/snapshots/tests__test@plan_cider.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@ cider-converter = $calyx-base/target/debug/cider-data-converter
rule run-cider-debug
command = $cider-exe -l $calyx-base --data data.dump $in debug || true
pool = console
converter-flags =
cider-flags =
rule run-cider
command = $cider-exe -l $calyx-base --data data.dump $in > $out
command = $cider-exe -l $calyx-base --data data.dump $cider-flags $in > $out
rule dump-to-interp
command = $cider-converter --to cider $in > $out
command = $cider-converter --to cider $converter-flags $in > $out
rule interp-to-dump
command = $cider-converter --to json $in > $out
command = $cider-converter --to json $converter-flags $in > $out
build data.dump: dump-to-interp $sim_data | $cider-converter

build interp_out.dump: run-cider /input.ext | data.dump
Expand Down
8 changes: 5 additions & 3 deletions fud2/tests/snapshots/tests__test@plan_debug.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@ cider-converter = $calyx-base/target/debug/cider-data-converter
rule run-cider-debug
command = $cider-exe -l $calyx-base --data data.dump $in debug || true
pool = console
converter-flags =
cider-flags =
rule run-cider
command = $cider-exe -l $calyx-base --data data.dump $in > $out
command = $cider-exe -l $calyx-base --data data.dump $cider-flags $in > $out
rule dump-to-interp
command = $cider-converter --to cider $in > $out
command = $cider-converter --to cider $converter-flags $in > $out
rule interp-to-dump
command = $cider-converter --to json $in > $out
command = $cider-converter --to json $converter-flags $in > $out
build data.dump: dump-to-interp $sim_data | $cider-converter

build /output.ext: run-cider-debug /input.ext | data.dump
Expand Down
107 changes: 100 additions & 7 deletions interp/src/errors.rs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
use crate::flatten::flat_ir::prelude::AssignedValue;
use crate::flatten::{
flat_ir::{
base::{AssignmentWinner, ComponentIdx, GlobalCellIdx, GlobalPortIdx},
prelude::AssignedValue,
},
structures::environment::Environment,
};
use crate::values::Value;
use calyx_ir::Id;
use calyx_utils::{Error as CalyxError, MultiError as CalyxMultiError};
Expand All @@ -17,6 +23,16 @@ impl BoxedInterpreterError {
pub fn inner_mut(&mut self) -> &mut InterpreterError {
&mut self.0
}

pub fn prettify_message<
C: AsRef<crate::flatten::structures::context::Context> + Clone,
>(
mut self,
env: &Environment<C>,
) -> Self {
self.0 = Box::new(self.0.prettify_message(env));
self
}
}

impl std::fmt::Display for BoxedInterpreterError {
Expand Down Expand Up @@ -112,6 +128,7 @@ pub enum InterpreterError {
"
)]
FlatConflictingAssignments {
target: GlobalPortIdx,
a1: AssignedValue,
a2: AssignedValue,
},
Expand Down Expand Up @@ -157,28 +174,32 @@ pub enum InterpreterError {
/// memory. Contains the name of the register or memory as a string
//TODO Griffin: Make this more descriptive
#[error(
"Attempted to write an undefined value to register or memory named \"{0}\""
"Attempted to write an undefined value to register or memory named \"{0:?}\""
)]
UndefinedWrite(String),
UndefinedWrite(GlobalCellIdx),

/// The error for attempting to write to an undefined memory address. This
/// is distinct from writing to an out of bounds address.
//TODO Griffin: Make this more descriptive
#[error(
"Attempted to write an undefined memory address in memory named \"{0}\""
"Attempted to write an undefined memory address in memory named \"{0:?}\""
)]
UndefinedWriteAddr(String),
UndefinedWriteAddr(GlobalCellIdx),

/// The error for attempting to read from an undefined memory address. This
/// is distinct from reading from an out of bounds address.
#[error(
"Attempted to read an undefined memory address from memory named \"{0}\""
"Attempted to read an undefined memory address from memory named \"{0:?}\""
)]
UndefinedReadAddr(String),
UndefinedReadAddr(GlobalCellIdx),

/// A wrapper for serialization errors
#[error(transparent)]
SerializationError(#[from] crate::serialization::SerializationError),

/// A nonspecific error, used for arbitrary messages
#[error("{0}")]
GenericError(String),
}

// this is silly but needed to make the program print something sensible when returning
Expand Down Expand Up @@ -206,3 +227,75 @@ impl From<std::str::Utf8Error> for InterpreterError {
CalyxError::invalid_file(err.to_string()).into()
}
}

impl InterpreterError {
pub fn prettify_message<
C: AsRef<crate::flatten::structures::context::Context> + Clone,
>(
self,
env: &Environment<C>,
) -> Self {
fn assign_to_string<C: AsRef<crate::flatten::structures::context::Context> + Clone>(
assign: &AssignedValue,
env: &Environment<C>,
) -> (
String,
Option<(ComponentIdx, crate::flatten::flat_ir::component::AssignmentDefinitionLocation)>,
){
match assign.winner() {
AssignmentWinner::Cell => ("Cell".to_string(), None),
AssignmentWinner::Implicit => ("Implicit".to_string(), None),
AssignmentWinner::Assign(idx) => {
let (comp, loc) =
env.ctx().find_assignment_definition(*idx);

let str = env.ctx().printer().print_assignment(comp, *idx);
(str, Some((comp, loc)))
}
}
}

fn source_to_string<
C: AsRef<crate::flatten::structures::context::Context> + Clone,
>(
source: &crate::flatten::flat_ir::component::AssignmentDefinitionLocation,
comp: ComponentIdx,
env: &Environment<C>,
) -> String {
let comp_name = env.ctx().lookup_name(comp);
match source {
crate::flatten::flat_ir::component::AssignmentDefinitionLocation::CombGroup(g) => format!(" in comb group {comp_name}::{}", env.ctx().lookup_name(*g)),
crate::flatten::flat_ir::component::AssignmentDefinitionLocation::Group(g) => format!(" in group {comp_name}::{}", env.ctx().lookup_name(*g)),
crate::flatten::flat_ir::component::AssignmentDefinitionLocation::ContinuousAssignment => format!(" in {comp_name}'s continuous assignments"),
//TODO Griffin: Improve the identification of the invoke
crate::flatten::flat_ir::component::AssignmentDefinitionLocation::Invoke(_) => format!(" in an invoke in {comp_name}"),
}
}

match self {
InterpreterError::FlatConflictingAssignments { target, a1, a2 } => {
let (a1_str, a1_source) = assign_to_string(&a1, env);
let (a2_str, a2_source) = assign_to_string(&a2, env);

let a1_v = a1.val();
let a2_v = a2.val();
let a1_source = a1_source
.map(|(comp, s)| source_to_string(&s, comp, env))
.unwrap_or_default();
let a2_source = a2_source
.map(|(comp, s)| source_to_string(&s, comp, env))
.unwrap_or_default();

let target = env.get_full_name(target);

InterpreterError::GenericError(
format!("conflicting assignments to port \"{target}\":\n 1. assigned {a1_v} by {a1_str}{a1_source}\n 2. assigned {a2_v} by {a2_str}{a2_source}")
)
}
InterpreterError::UndefinedWrite(c) => InterpreterError::GenericError(format!("Attempted to write an undefined value to register or memory named \"{}\"", env.get_full_name(c))),
InterpreterError::UndefinedWriteAddr(c) => InterpreterError::GenericError(format!("Attempted to write to an undefined memory address in memory named \"{}\"", env.get_full_name(c))),
InterpreterError::UndefinedReadAddr(c) => InterpreterError::GenericError(format!("Attempted to read from an undefined memory address from memory named \"{}\"", env.get_full_name(c))),
e => e,
}
}
}
11 changes: 11 additions & 0 deletions interp/src/flatten/flat_ir/base.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,17 @@ pub enum AssignmentWinner {
Assign(AssignmentIdx),
}

impl AssignmentWinner {
#[must_use]
pub fn as_assign(&self) -> Option<&AssignmentIdx> {
if let Self::Assign(v) = self {
Some(v)
} else {
None
}
}
}

impl From<AssignmentIdx> for AssignmentWinner {
fn from(v: AssignmentIdx) -> Self {
Self::Assign(v)
Expand Down
99 changes: 99 additions & 0 deletions interp/src/flatten/flat_ir/component.rs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use super::super::structures::context::Context;
use crate::flatten::structures::{
index_trait::{IndexRange, SignatureRange},
indexed_map::IndexedMap,
Expand Down Expand Up @@ -71,6 +72,104 @@ pub struct ComponentCore {
pub done: LocalPortOffset,
}

pub enum AssignmentDefinitionLocation {
/// The assignment is contained in a comb group
CombGroup(CombGroupIdx),
/// The assignment is contained in a group
Group(GroupIdx),
/// The assignment is one of the continuous assignments for the component
ContinuousAssignment,
/// The assignment is implied by an invoke
Invoke(ControlIdx),
}

impl ComponentCore {
/// Returns true if the given assignment is contained in this component.
///
/// Note: This is not a very efficient implementation since it's doing a
/// DFS search over the control tree.
pub fn contains_assignment(
&self,
ctx: &Context,
assign: AssignmentIdx,
) -> Option<AssignmentDefinitionLocation> {
if self.continuous_assignments.contains(assign) {
return Some(AssignmentDefinitionLocation::ContinuousAssignment);
} else if let Some(root) = self.control {
let mut search_stack = vec![root];
while let Some(node) = search_stack.pop() {
match &ctx.primary[node] {
ControlNode::Empty(_) => {}
ControlNode::Enable(e) => {
if ctx.primary[e.group()].assignments.contains(assign) {
return Some(AssignmentDefinitionLocation::Group(
e.group(),
));
}
}
ControlNode::Seq(s) => {
for stmt in s.stms() {
search_stack.push(*stmt);
}
}
ControlNode::Par(p) => {
for stmt in p.stms() {
search_stack.push(*stmt);
}
}
ControlNode::If(i) => {
if let Some(comb) = i.cond_group() {
if ctx.primary[comb].assignments.contains(assign) {
return Some(
AssignmentDefinitionLocation::CombGroup(
comb,
),
);
}
}

search_stack.push(i.tbranch());
search_stack.push(i.fbranch());
}
ControlNode::While(wh) => {
if let Some(comb) = wh.cond_group() {
if ctx.primary[comb].assignments.contains(assign) {
return Some(
AssignmentDefinitionLocation::CombGroup(
comb,
),
);
}
}
search_stack.push(wh.body());
}
ControlNode::Repeat(r) => {
search_stack.push(r.body);
}
ControlNode::Invoke(i) => {
if let Some(comb) = i.comb_group {
if ctx.primary[comb].assignments.contains(assign) {
return Some(
AssignmentDefinitionLocation::CombGroup(
comb,
),
);
}
}

if i.assignments.contains(assign) {
return Some(AssignmentDefinitionLocation::Invoke(
node,
));
}
}
}
}
}
None
}
}

#[derive(Debug, Clone)]
/// Other information about a component definition. This is not on the hot path
/// and is instead needed primarily during setup and error reporting.
Expand Down
Loading

0 comments on commit ce58e29

Please sign in to comment.