Skip to content

Conversation

@vidsinghal
Copy link
Collaborator

@vidsinghal vidsinghal commented Feb 12, 2024

WIP: Add a pass to analyze functions that are tail recursive and add mutable cursors.

In case of a mutable cursor, we do not need to store a (Cursor, Cursor) for the start 
and the end of a packed value. 
In case a location referes to a mutable cursor, we can just keep a Mutable Cursor for
that location. 

Mutable Cursor version for add1Tree

-- char* 
type Cursor = Ptr Char 

-- char**
type MutableCursor = Ptr Ptr Char

add1 :: Cursor -> Cursor -> ()
add1 lout lin = 
  let tag = readTag lin 
   in case tag of 
        Leaf  -> let n = readInt (tag + 1) 
                     () = writeTagMutable lout Leaf
                     () = BumpMutableCursor lout 1  
                     () = writeIntMutable lout (n+1)
                     () = BumpMutableCursor lout 8
                   in ()
        Node -> ...

Packed input still becomes a read cursor. 
Packed output values become just one mutable cursor.

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a temporary change to see how far CI advances, or do you intend it to stay that way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just temporary

Comment on lines +5 to +7



Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But better yet, you should simply use record update syntax here: funTy { tailRecType = TC } -- instead of the whole let.

Comment on lines 33 to 36
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what other branch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 x in a value y to z, you write y { x = z }

https://en.wikibooks.org/wiki/Haskell/More_on_datatypes#Named_Fields_(Record_Syntax)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh ok conditional branch, branch was ambiguous at first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, my bad!

@vidsinghal vidsinghal linked an issue Mar 30, 2025 that may be closed by this pull request
10 tasks
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.

[SoA Backend]: Implementation progress issue

3 participants