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

push! doesn't work on BBs #57

Open
femtomc opened this issue Apr 29, 2020 · 2 comments
Open

push! doesn't work on BBs #57

femtomc opened this issue Apr 29, 2020 · 2 comments

Comments

@femtomc
Copy link

femtomc commented Apr 29, 2020

Not sure if this is intended but https://github.com/MikeInnes/IRTools.jl/blob/bacecb8a71eb0026963021d33e4ba16bfc7211da/src/ir/ir.jl#L630 doesn't work on BBs, as illustrated:

function block_transform(ir)
    ir = copy(ir)
    for (i, bb) in enumerate(ir.blocks)
        for (j, stmt) in enumerate(bb.stmts)
            stmt.expr.head == :call && 
            stmt.expr.args[1] isa GlobalRef && 
            stmt.expr.args[1].name == :rand && 
            begin   
                new = argument!(ir)
                new_stmt = IRTools.Inner.Statement(
                                          Expr(:call, 
                             GlobalRef(stmt.expr.args[1].mod, :logpdf),
                             stmt.expr.args[2:length(stmt.expr.args)]...,
                             new),
                                          stmt.type,
                                          stmt.line)
                v = push!(bb, new_stmt)
            end
        end
    end
    return renumber(ir)
end

which throws

ERROR: LoadError: MethodError: no method matching push!(::IRTools.Inner.BasicBlock, ::IRTools.Inner.Statement)

because there's no inheritance
https://github.com/MikeInnes/IRTools.jl/blob/bacecb8a71eb0026963021d33e4ba16bfc7211da/src/ir/ir.jl#L129-L134

It looks like it's easy to implement this i.e. in the /src for push!:

push!(BasicBlock(b).stmts, x)
push!(b.ir.defs, (b.id, length(BasicBlock(b).stmts)))
return Variable(length(b.ir.defs))

I think this issue is about a more general confusion of the difference between Block and BasicBlock (i.e. the documentation uses block to possibly refer to both?)

@femtomc
Copy link
Author

femtomc commented Apr 29, 2020

Ah! You must use blocks instead of fieldname indexing with .blocks to get a Block - I didn't realize that.

@MikeInnes
Copy link
Member

Yup – Block is the public interface to all block operations, whereas BasicBlock is just the internal representation. Perhaps we need to be clear in the docs that internal fields are off-limits.

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

No branches or pull requests

2 participants