FluxML / IRTools.jl

Mike's Little Intermediate Representation
MIT License
111 stars 36 forks source link

push! doesn't work on BBs #57

Open femtomc opened 4 years ago

femtomc commented 4 years ago

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 commented 4 years ago

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

MikeInnes commented 4 years ago

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.