cucapra / filament

Fearless hardware design
http://filamenthdl.com/
MIT License
160 stars 9 forks source link

Remove dependence on specific `Info`s #462

Open UnsignedByte opened 1 month ago

UnsignedByte commented 1 month ago

@ethanuppal has mentioned issues that arise when every info is set to empty. This is something that arises from the info_cast macro defined in info.rs: https://github.com/cucapra/filament/blob/1f9525419ecad50e2ef6c44514939f654c29fc82/crates/ir/src/info.rs#L264-L298 It defines the From trait for infos, which panics if the incorrect info type is found. This is used in certain locations primarily to generate ir::Info::Reasons for constraints (some listed below). These should theoretically just return Info::Empty if either of the parents is missing, rather than panicking, so that we could remove the dependency on info entirely throughout the IR.

We should instead implement TryFrom or instead just directly force the user to use the as_type functions that are already defined.

Non-exhaustive examples

https://github.com/cucapra/filament/blob/1f9525419ecad50e2ef6c44514939f654c29fc82/crates/filament/src/ir_passes/type_check.rs#L28-L36 https://github.com/cucapra/filament/blob/1f9525419ecad50e2ef6c44514939f654c29fc82/crates/filament/src/ir_passes/interval_check.rs#L91-L112

rachitnigam commented 1 month ago

Hm, some of these are tricky because they are not quite "info" (i.e., optional things that you are allowed to forget)...

UnsignedByte commented 1 month ago

What do you mean by this? I was under the assumption that the ir::Info::Reasons were used purely for pretty printing diagnostics if they occurred, and that the assumptions added could still be done with Info::Empty instead?

rachitnigam commented 1 month ago

The two examples you quoted have info::EventBind and info::Port

UnsignedByte commented 1 month ago

Yes but those are just used to get bind_locs to generate the other infos, so are not really necessary either (just propogate empties or loc::unknown)

ethanuppal commented 3 weeks ago

In my code I was pretty safe to just fill dummy infos with no location — that being said, I couldn't figure out how to actually get invocations working, so maybe invocations have a legitimate dependency on AST location info