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

Implement alias scopes #31018

Merged
merged 1 commit into from Feb 24, 2019
Merged

Implement alias scopes #31018

merged 1 commit into from Feb 24, 2019

Conversation

YingboMa
Copy link
Contributor

@YingboMa YingboMa commented Feb 9, 2019

This PR is based on the original PR #20257 by @Keno.

struct Const{T<:Array}
    a::T
end

@eval Base.getindex(A::Const, i1::Int) = Core.const_arrayref($(Expr(:boundscheck)), A.a, i1)
@eval Base.getindex(A::Const, i1::Int, i2::Int, I::Int...) =  (Base.@_inline_meta; Core.const_arrayref($(Expr(:boundscheck)), A.a, i1, i2, I...))

macro aliasscope(body)
    sym = gensym()
    esc(quote
        $(Expr(:aliasscope))
        $sym = $body
        $(Expr(:popaliasscope))
        $sym
    end)
end

using BenchmarkTools, Test
function micro_ker_const!(AB, Ac::Vector{Float64}, Bc::Vector{Float64}, kc::Int, offSetA::Int, offSetB::Int)
    MR = 8; NR = 6;
    @inbounds @aliasscope for k in 1:kc
        for j in 1:NR, i in 1:MR
            AB[i+(j-1)*MR] = muladd(Const(Ac)[offSetA+i], Const(Bc)[offSetB+j], Const(AB)[i+(j-1)*MR])
        end
        offSetA += MR
        offSetB += NR
    end
    return
end
function micro_ker!(AB, Ac::Vector{Float64}, Bc::Vector{Float64}, kc::Int, offSetA::Int, offSetB::Int)
    MR = 8; NR = 6;
    @inbounds for k in 1:kc
        for j in 1:NR, i in 1:MR
            AB[i+(j-1)*MR] = muladd(Ac[offSetA+i], Bc[offSetB+j], AB[i+(j-1)*MR])
        end
        offSetA += MR
        offSetB += NR
    end
    return
end
AB = zeros(8, 6); Ac = rand(8*32); Bc = rand(32*6);
micro_ker!(AB, Ac, Bc, 32, 0, 0)
@test reshape(Ac, 8, 32) * reshape(Bc, 6, 32)'  reshape(AB, 8, 6)
AB .= 0
micro_ker_const!(AB, Ac, Bc, 32, 0, 0)
@test reshape(Ac, 8, 32) * reshape(Bc, 6, 32)'  reshape(AB, 8, 6)
@btime micro_ker_const!($AB, $Ac, $Bc, 32, 0, 0)
@btime micro_ker!($AB, $Ac, $Bc, 32, 0, 0)
julia> @btime micro_ker_const!($AB, $Ac, $Bc, 32, 0, 0)
  231.211 ns (0 allocations: 0 bytes)

julia> @btime micro_ker!($AB, $Ac, $Bc, 32, 0, 0)
  863.300 ns (0 allocations: 0 bytes)
julia> @code_llvm optimize=false raw=true dump_module=true micro_ker_const!(AB, Ac, Bc, 32, 0, 0)
...
    %34 = load double, double addrspace(13)* %33, align 8, !dbg !40, !tbaa !48, !alias.scope !51
...
    %46 = load double, double addrspace(13)* %45, align 8, !dbg !40, !tbaa !48, !alias.scope !51
...
    %60 = load double, double addrspace(13)* %59, align 8, !dbg !40, !tbaa !48, !alias.scope !51
...
    store double %62, double addrspace(13)* %75, align 8, !dbg !61, !tbaa !48, !noalias !51
...
!51 = !{!52}
!52 = !{!"aliasscope", !53}

CC: @vchuravy

@YingboMa
Copy link
Contributor Author

YingboMa commented Feb 9, 2019

Related issues: #19658 #8087 #28126 #29026

@vchuravy
Copy link
Sponsor Member

vchuravy commented Feb 9, 2019

@maleadt we should probably talk about how we could extend this for CUDAnative. Right now it is based on a compiler intrinsic.

Thanks Yingbo, for hacking on this together!

std::vector<MDNode*> scope_list_stack;
{
size_t nstmts = jl_array_len(src->code);
aliasscopes.resize(nstmts + 1, nullptr);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This block and the for-loop the part that Yingbo and I were the most unsure about. We mimicked the + 1 from the linetable construct above.

current_aliasscope = scope_list_stack.back();
}
}
aliasscopes[i+1] = current_aliasscope;
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Same with the +1 here.

@YingboMa YingboMa force-pushed the vc/aliasscopes branch 2 times, most recently from 4059af7 to 63fe6cf Compare February 9, 2019 02:50
src/builtins.c Outdated
@@ -1210,6 +1215,7 @@ void jl_init_primitives(void) JL_GC_DISABLED

// array primitives
add_builtin_func("arrayref", jl_f_arrayref);
add_builtin_func("const_arrayref", jl_f_const_arrayref);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

#20257 (comment)

could just call jl_f_arrayref here?

@Keno
Copy link
Member

Keno commented Feb 9, 2019

FWIW, I remain unconvinced that this is the correct long-term solution, but I of course recognize the short term utility here, so we should perhaps figure out a way to expose this without boxing ourself in with this as a support API (e.g. perhaps add the intrinsics, but only add the supporting definitions in a package that is explicitly disclaimed as unsupported).

@vchuravy
Copy link
Sponsor Member

I agree with that, this "feels" a bit odd and I wouldn't want us to have to live with it forever. Right now this PR is relatively minimal and doesn't expose anything besides the AST nodes and the intrinsic.
I would love to have this for some stuff Peter and I are working on, but I am fine with declaring the experimental and unsupported.

@StefanKarpinski
Copy link
Sponsor Member

It seems very naively to me like what might be needed is the ability to tell the compiler that this is a good point to check if a set of arrays alias each other and allow the compiler to generate code assuming both positive and negative results. That way you get fast behavior in the good case and correct behavior no matter what.

@vchuravy
Copy link
Sponsor Member

This is what LLVM does under the hood right now. Sometimes it fails because it can't infer the array bounds (so doing it at the Julia level might help) and sometimes you want to avoid the code size increase. We have unalias now so it should be more straightforward to write valid code and not shoot yourself in the foot.

Co-authored-by: Keno Fischer <kfischer@college.harvard.edu>
Co-authored-by: Yingbo Ma <mayingbo5@gmail.com>
Co-authored-by: Valentin Churavy <v.churavy@gmail.com>
@YingboMa
Copy link
Contributor Author

The CI error seems unrelated
https://travis-ci.org/JuliaLang/julia/jobs/497227682#L5559

failed to clone from https://github.com/JuliaLang/Example.jl.git, error: GitError(Code:ERROR, Class:Net, curl error: Could not resolve host: github.com

@vchuravy
Copy link
Sponsor Member

@Keno are you okay with merging this as is?

@StefanKarpinski
Copy link
Sponsor Member

Question: is this change purely internal or is it a public API? It doesn't add or change any exports, so I'm assuming it's internal not public. In that case, it seems fine to merge it as an experimental feature to learn how to deal with aliasing better.

@Keno
Copy link
Member

Keno commented Feb 23, 2019

I'm ok with this on the understanding that it will definitely go away in the future.

@vchuravy
Copy link
Sponsor Member

Completely internal and all I interested parties are okay with this going away :)

@vchuravy vchuravy merged commit 7e4641d into JuliaLang:master Feb 24, 2019
@YingboMa YingboMa deleted the vc/aliasscopes branch March 11, 2019 00:06
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.

None yet

4 participants