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

IRBuilder and recursive function #181

Closed
ghost opened this issue Mar 3, 2018 · 5 comments
Closed

IRBuilder and recursive function #181

ghost opened this issue Mar 3, 2018 · 5 comments

Comments

@ghost
Copy link

@ghost ghost commented Mar 3, 2018

If I use function from IRBuilder.Module and refer to it inside the function itself (recursion) the code hangs in an infinite loop.

How does one do recursion with IRBuilder?

@cocreature
Copy link
Member

@cocreature cocreature commented Mar 4, 2018

This looks like a bug, here’s a minimal example to reproduce this

{-# LANGUAGE RecursiveDo #-}
{-# LANGUAGE OverloadedStrings #-}

module Main where

import Text.Show.Pretty (pPrint)

import LLVM.AST hiding (function)
import LLVM.AST.Type as AST
import qualified LLVM.AST.Float as F
import qualified LLVM.AST.Constant as C

import LLVM.IRBuilder.Module
import LLVM.IRBuilder.Monad
import LLVM.IRBuilder.Instruction

simple :: Module
simple = buildModule "exampleModule" $ mdo

  bar <- function "bar" [(AST.i32, "a")] AST.i32 $ \[a] -> mdo
    entry <- block `named` "entry"; do
      ret a

  foo <- function "foo" [(AST.i32, "a")] AST.i32 $ \[a] -> mdo
    entry <- block `named` "entry"; do
      c <- call foo [(a, [])]
      ret c

  pure ()


main :: IO ()
main = pPrint simple

It works if we change ModuleBuilderT to use a lazy state monad in

newtype ModuleBuilderT m a = ModuleBuilderT { unModuleBuilderT :: StateT ModuleBuilderState m a }
. However, I haven’t figured out why this is necessary so far and I’d like to understand this before we commit this. So if anyone has any ideas, I’m all ears.

Loading

@cocreature
Copy link
Member

@cocreature cocreature commented Mar 4, 2018

Alright, here’s a minimal example to demonstrate the problem: https://gist.github.com/cocreature/c05107723b9882d992534304a9a491e8

The strict pattern match in our case is caused by

There are two solutions here:

  1. Use the lazy state monad. This requires no changes to existing code but could introduce space leaks in some cases. Since pretty much all operations also grow the size of the module, I suspect that this would only be a relatively small constant factor increase.
  2. Have separate instructions for call and callVoid and thereby remove the need for the case statement. The downside is that the obligation for using the correct version is on the users, the upside is that we can give callVoid a m () return type instead of m Operand and thereby prevent users from using the result of a call to a void function.

Personally, I don’t have any particular preference here. Maybe @ollef and @sdiehl have opinions on this.

Loading

@ollef
Copy link
Member

@ollef ollef commented Mar 5, 2018

Good find!

Both solutions sound fine to me.

Loading

@cocreature cocreature closed this in b7f2bcd Mar 6, 2018
@cocreature
Copy link
Member

@cocreature cocreature commented Mar 6, 2018

I’ve gone with option 1 for now, we can always revisit this later if it becomes a problem. I’ll try to get a release out later today (LLVM 6 has tagged a final release).

Loading

@sdiehl
Copy link
Member

@sdiehl sdiehl commented Mar 6, 2018

All looks good to me.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants