Tail recursion#254
Conversation
ulysses4ever
left a comment
There was a problem hiding this comment.
Cool work, congrats!
Can you remove the binary (*.exe) files from the patch? Perhaps, the .s assembly file too. They can be generated, right? And they have a high chance of failing on a machine other than yours. Hence, they should not be committed.
There should be a test checking that TCO happens in a simple example. Let me know if you need help creating such a test: we can discuss and find a way: worst case, we can grep the assembly. We just need to figure out where to fit this in the existing test suite.
|
|
||
| program-options | ||
| ghc-options: -Werror | ||
| ghc-options: |
There was a problem hiding this comment.
Is this a temporary change to see how far CI advances, or do you intend it to stay that way?
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Can we have one empty line between definitions? (Same below.)
| let tailCallTy = markTailCallsFnBody funName ddefs funTy funBody | ||
| if elem TC tailCallTy | ||
| then | ||
| let (ArrowTy2 locVars arrIns _arrEffs arrOut _locRets _isPar _) = funTy |
There was a problem hiding this comment.
The last _ should follow the same convention as other unused binding, i.e. _blah where blah gives some meaning to the binding. Same below
| if elem TC tailCallTy | ||
| then | ||
| let (ArrowTy2 locVars arrIns _arrEffs arrOut _locRets _isPar _) = funTy | ||
| funTy' = (ArrowTy2 locVars arrIns _arrEffs arrOut _locRets _isPar TC) |
There was a problem hiding this comment.
You're using binding names with the _ prefix, which are usually meant to be unused: _blah is how you give a name to a binding otherwise unused to improve readability and avoid the unused-binding warning. I suggest removing _ from the names that are actually used. There was an old GHC ticket that suggested making this a warning but it didn't get traction.
There was a problem hiding this comment.
But better yet, you should simply use record update syntax here: funTy { tailRecType = TC } -- instead of the whole let.
| then | ||
| let (ArrowTy2 locVars arrIns _arrEffs arrOut _locRets _isPar _) = funTy | ||
| funTy' = (ArrowTy2 locVars arrIns _arrEffs arrOut _locRets _isPar TMC) | ||
| in dbgTraceIt (sdoc (tailCallTy, funName, funTy')) dbgTraceIt "b" return $ FunDef funName funArgs funTy' funBody funMeta |
There was a problem hiding this comment.
This branch looks very similar to the previous one. I suggest factoring out common code. It's very hard to spot a difference and hence, to miss a typo.
There was a problem hiding this comment.
You got to this comment after modifying the code, so it's tricky. The same duplication is still in the current version here: https://github.com/iu-parfunc/gibbon/blob/cef8b0d0296aa382579d8fc9a0d044f6b2b169dc/gibbon-compiler/src/Gibbon/Passes/MarkTailCalls.hs#L34-L40
Are you aware of the record update syntax in Haskell? E.g.:
So, in general, to update the field
xin a valueytoz, you writey { x = z }
https://en.wikibooks.org/wiki/Haskell/More_on_datatypes#Named_Fields_(Record_Syntax)
There was a problem hiding this comment.
Oh ok conditional branch, branch was ambiguous at first.
… decoration of tail call info
… for additional location metadata
This reverts commit adfb1ac.
WIP: Add a pass to analyze functions that are tail recursive and add mutable cursors.