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

Compile Prim nodes to Hugr DFG #34

Merged
merged 9 commits into from
Oct 21, 2024
Merged

Compile Prim nodes to Hugr DFG #34

merged 9 commits into from
Oct 21, 2024

Conversation

acl-cqc
Copy link
Collaborator

@acl-cqc acl-cqc commented Oct 4, 2024

  • The first couple of examples/files where I looked to put this seemed misnamed, so some driveby cleanups
  • The test I've added is monomorphic, as anything polymorphic fails validation due to type erasure (Type Erasure, Dynamic, Coercions #18)....and this seems like one where we really want validation to succeed to tell us the rest is right :)

@acl-cqc acl-cqc requested a review from croyzor October 4, 2024 17:48
@@ -0,0 +1,15 @@
ext "to_float" to_float(i :: Int) -> Float
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe this file would be better called "mono_vec_map.brat" (or monomorphic...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also "list" rather than "vec"

ins <- addNodeWithInputs ("Inputs" ++ n) (OpIn (InputNode dfg_id inputTys)) [] inputTys
outs <- addNodeWithInputs n (OpCustom (CustomOp dfg_id ext op box_sig [])) ins outputTys
addNodeWithInputs ("Outputs" ++ n) (OpOut (OutputNode dfg_id outputTys)) outs [] >>= \case
[] -> pure ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the test that there's no outputs seems superfluous (+ haskell will emit a warning about the incomplete pattern match)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, not superfluous exactly, but I don't think we need to be quite so defensive

Copy link
Collaborator Author

@acl-cqc acl-cqc Oct 15, 2024

Choose a reason for hiding this comment

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

Yeah, this is pretty much the same thing as your other comment - it does seem dang hard to write what are basically assertions, so instead we just drop the value and lose a (small in terms of source code) check that things are working the way we expect. And the warnings against incomplete matches (there are loads in this file!) just require us to write much longer/more verbose code to turn one kind of runtime failure into another...

What's the right thing to do here? Can we instance MonadFail Compile and handle all these incomplete matches in our own code (which might still just error but at least it'd silence the warnings)? For another PR, I mean - I've shortened/silenced these here for the time being.

addNodeWithInputs ("Outputs" ++ n) (OpOut (OutputNode dfg_id outputTys)) outs [] >>= \case
[] -> pure ()
case p of
(Port loadConst 0) -> pure $ default_edges loadConst
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for the sake of the GHC warning, we should add a catch all that throws an error

@acl-cqc acl-cqc merged commit 462a480 into main Oct 21, 2024
1 check passed
@acl-cqc acl-cqc deleted the feat/compile_prim branch October 21, 2024 09:28
croyzor pushed a commit that referenced this pull request Oct 24, 2024
* Driveby cleanups of some misnamed `examples/`
* Add monomorphic test so it compiles+validaties rather than having any type erasure issues (#18)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants