search

IntelLabs / ParallelAccelerator.jl

The ParallelAccelerator package, part of the High Performance Scripting project at Intel Labs
BSD 2-Clause "Simplified" License
294 stars 32 forks source link

@par array access bug #132

Closed ehsantn closed 7 years ago

ehsantn commented 7 years ago

Parallel array accesses in @par (cartesianmapreduce) are translated to getindex() but they should be unsafe_arrayref(). Also, they are not in read/write set of parfors.

using ParallelAccelerator

@acc function score2()
    samples = Float64[i for i in 1:6]
    points = [-1.0, 2.0, 5.0]
    b = 3.0
    N = size(points,1)
    exps = 0.0
    @par exps(+) for i in 1:length(samples)
       d = -(samples[i]-points).^2/(2*b.^2)
       exps += minimum(d)-log(b*N)+log(sum(exp(d-minimum(d))))
   end
   return exps
end

Comprehensions (cartesianarray) has the correct behavior.

ninegua commented 7 years ago

The accesses should be getindex, because we can't be sure sample[i] is safe. We don't automatically convert getindex to unsafe_arrayref, since we don't infer the range of indices. The array size equivalence is only used for the purpose of fusion. The same is true for cartesianarray.

That being said, we are currently translating getindex directly to ARRAYELEM in CGen, the same as unsafe_arrayref. So in the end it doesn't matter. I personally think this is sloppy since original J2C did it the right way, but on the other hand, one can argue that @acc should assume all indexing are inbound.

I don't know much about the read/write sets. Maybe @DrTodd13 can take a look here.

DrTodd13 commented 7 years ago

ParallelIR RWS callback wasn't converting getindex so that it looked like an arrayref to RWS. I have added this and had to change RWS itself to process the callback first rather than a last ditch effort to account for this case where a comprehensible call needs to be transformed to a different call in RWS to get the right effect. This should fix the RWS issue I hope.

However, with this particular example, I see the following. Anybody else can confirm?

ERROR: LoadError: UndefVarError: ret not defined in score2() at /home/taanders/.julia/v0.5/CompilerTools/src/OptFramework.jl:598 in include_from_node1(::String) at ./loading.jl:488 in process_options(::Base.JLOptions) at ./client.jl:262 in _start() at ./client.jl:318 while loading /tmp/t1.jl, in expression starting on line 20

ehsantn commented 7 years ago

exps needs a type. Can we have a meaningful error thrown when a type for an escaping variable is necessary?

DrTodd13 commented 7 years ago

Indeed. Adding a type to exps outside the @par fixes the problem. I think we should close this issue and open another one.

ehsantn commented 7 years ago

The RWS issue is resolved (I just double-checked).