Add tuple return values for par exprs in codegen#157
Open
EmilySillars wants to merge 3 commits intomainfrom
Open
Add tuple return values for par exprs in codegen#157EmilySillars wants to merge 3 commits intomainfrom
EmilySillars wants to merge 3 commits intomainfrom
Conversation
EmilySillars
commented
Apr 28, 2023
Comment on lines
+814
to
+832
| -- given a list of par return vals and their types, wrap the return vals in a tuple | ||
| genParRetVal :: [C.Exp] -> [I.Type] -> GenFn (C.Exp, [C.BlockItem]) | ||
| genParRetVal [] _ = fail "par should have 2 or more return values" | ||
| genParRetVal [_] _ = fail "par should have 2 or more return values" | ||
| genParRetVal rets@(ret0 : ret1 : _) retTys = do | ||
| -- TODO: given n ret vals, return an n-ary tuple | ||
| when (length rets /= length retTys) $ do fail "lists of return vals and types must be same length" | ||
| let dcon = I.tempTupleId (length rets) | ||
| let dty = I.tempTuple retTys | ||
| onHeap <- getsDCon dconOnHeap dcon | ||
| unless onHeap $ do | ||
| fail $ "Cannot handle packed fields yet, for: " ++ show dcon | ||
| construct <- getsDCon dconConstruct dcon | ||
| destruct <- getsDCon dconDestruct dcon | ||
| tmp <- genTmp dty | ||
| let alloc = [[citem|$exp:tmp = $exp:construct;|]] | ||
| initField y i = [citem|$exp:(destruct i tmp) = $exp:y;|] | ||
| initFields = zipWith initField [ret0, ret1] [0 ..] -- puts first two return vals in a 2-tuple for now | ||
| return (tmp, alloc ++ initFields) |
Contributor
Author
There was a problem hiding this comment.
creates a 2-tuple with the first two ret vals of par as its arguments
j-hui
reviewed
May 4, 2023
Contributor
j-hui
left a comment
j-hui
left a comment
There was a problem hiding this comment.
Here's a quick review! But I have the following questions:
- Why isn't codegen for more than 2 tuples supported? It doesn't seem like the logic is any different from regular tuples.
- Why do we need to define
type Pair2 a bin the source code, and why is it written as that (instead of(a, b))?
Otherwise, the code looks really good, but those questions need to be answered/addressed before we can merge this in.
| @@ -1,3 +1,6 @@ | |||
| type Pair2 a b | |||
Contributor
There was a problem hiding this comment.
Why was this not there before? And is it being used?
| import Control.Monad (foldM, unless) | ||
| import Control.Monad (foldM, unless, when) | ||
|
|
||
| -- import Control.Monad (foldM, unless) |
Contributor
There was a problem hiding this comment.
Remove the commented-out import
| fail $ "Cannot compile par with non-application expression: " ++ show e | ||
| -- given a list of par return vals and their types, wrap the return vals in a tuple | ||
| genParRetVal :: [C.Exp] -> [I.Type] -> GenFn (C.Exp, [C.BlockItem]) | ||
| genParRetVal [] _ = fail "par should have 2 or more return values" |
Contributor
There was a problem hiding this comment.
nit: You can just put this as a _ wildcard case after the rets@(ret0:ret1:_) case, to consolidate them
| genParRetVal [_] _ = fail "par should have 2 or more return values" | ||
| genParRetVal rets@(ret0 : ret1 : _) retTys = do | ||
| -- TODO: given n ret vals, return an n-ary tuple | ||
| when (length rets /= length retTys) $ do fail "lists of return vals and types must be same length" |
Contributor
There was a problem hiding this comment.
Put the fail statement on the line after the do, i.e.,
when (length rets /= length retTys) $ do
fail "lists of return vals and types must be same length"| -- TODO: given n ret vals, return an n-ary tuple | ||
| when (length rets /= length retTys) $ do fail "lists of return vals and types must be same length" | ||
| let dcon = I.tempTupleId (length rets) | ||
| let dty = I.tempTuple retTys |
Contributor
There was a problem hiding this comment.
Remove the redundant let here, should just be:
let dcon = ...
dty = ...| tmp <- genTmp dty | ||
| let alloc = [[citem|$exp:tmp = $exp:construct;|]] | ||
| initField y i = [citem|$exp:(destruct i tmp) = $exp:y;|] | ||
| initFields = zipWith initField [ret0, ret1] [0 ..] -- puts first two return vals in a 2-tuple for now |
Contributor
There was a problem hiding this comment.
Why only the first two values? Why not zipWith initField rets [0..]?
Contributor
|
FYI 8d50b53 merged in support for tuples without having to define the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR modifies code generation so that the return values of par expressions are saved in a 2-tuple.