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

chore: Upgrade to hugr 0.8.0 #66

Merged
merged 6 commits into from
Dec 17, 2024
Merged

chore: Upgrade to hugr 0.8.0 #66

merged 6 commits into from
Dec 17, 2024

Conversation

croyzor
Copy link
Collaborator

@croyzor croyzor commented Dec 12, 2024

Resolves #63

@@ -201,7 +200,7 @@ compileConst parent tm ty = do
constId <- addNode "Const" (OpConst (ConstOp parent (valFromSimple tm)))
loadId <- case ty of
HTFunc poly@(PolyFuncType [] _) ->
addNode "LoadFunction" (OpLoadFunction (LoadFunctionOp parent poly [] (FunctionType [] [HTFunc poly])))
addNode "LoadFunction" (OpLoadFunction (LoadFunctionOp parent poly [] (FunctionType [] [HTFunc poly] [])))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are the extension reqs for LoadFunction empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LoadFunction is only used for const functions i thought? The extensions in the function that's loaded are boxed up until it is called

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no idea, just checking 😅

But that makes sense 👍

@@ -295,13 +294,14 @@ compileClauses parent ins ((matchData, rhs) :| clauses) = do
didntMatch outTys parent ins = case nonEmpty clauses of
Just clauses -> compileClauses parent ins clauses
-- If there are no more clauses left to test, then the Hugr panics
Nothing -> let sig = FunctionType (snd <$> ins) outTys in
addNodeWithInputs "Panic" (OpCustom (CustomOp parent "brat" "panic" sig [])) ins outTys
-- TODO: This should just be [BRAT] - would that work?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't ops add their own extension as a requirement implicitly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not as part of the signature - I've implemented this comment anyway

@@ -437,7 +438,7 @@ compileWithInputs parent name = gets compiled >>= (\case
_ -> compileWithInputs parent outNode >>= \case
Just calleeId -> do
callerId <- addNode ("indirect_call(" ++ show calleeId ++ ")")
(OpCallIndirect (CallIndirectOp parent (FunctionType ins outs)))
(OpCallIndirect (CallIndirectOp parent (FunctionType ins outs bratExts {-[]-})))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be empty or not, c.f. LoadFunction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The extensions here should match the extensions of the callee, but we're overapproximating

Comment on lines 826 to 827
-- HELP! This doesn't work at all when extensions are involved!!!
--
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still looking for help? What's going wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only a problem when we stop overapproximating extensions, I'll zap the comment

Comment on lines 19 to 26
bratExts =
["prelude"
,"arithmetic.int_ops"
,"arithmetic.float_ops"
,"collections"
,"logic"
,"tket2.quantum"
,"BRAT"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

arithmetic.int_types and arithmetic.float_types are missing

args = [funcToSeq outerSig, TASequence (funcToSeq <$> innerSigs)]

funcToSeq (FunctionType ins outs) = TASequence [toSeq ins, toSeq outs]
sig = FunctionType (toFunc <$> (outerSig : innerSigs)) [toFunc outerSig] ["BRAT"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we also need to include the reqs of the input graphs?

@croyzor croyzor requested a review from mark-koch December 17, 2024 16:03
@croyzor croyzor merged commit f0c22a4 into main Dec 17, 2024
2 checks passed
@croyzor croyzor deleted the chore/hugr-v0.8.0 branch December 17, 2024 17:12
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.

Upgrade hugr to 0.8.0
2 participants