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

Error: "Conflicting variable x exists in both inner and outer lambda" #103

Closed jihoonseo closed 8 years ago

jihoonseo commented 8 years ago

Hi I've been trying to apply ParallelAccelerator to my code, and some errors aroused. (I am not sure whether there will exist some performance gain with parallelization or not, and if someone points that there will not, then I will not use parallelization on this function.)

de2bi() is a function that converts a decimal number to a binary array that LSB (least significant bit) is on the left.

FYI: You may test de2bi() with decimal = 10, pad = 4, like de2bi(10, 4) -> [0 1 0 1].

using ParallelAccelerator

@acc function de2bi(decimal, pad)

  #= v1
  binaryString = bin(decimal, pad)
  binaryStringReversed = reverse(binaryString)

  charArray = collect(binaryStringReversed)

  intArray = Array{Int64}(1, pad)

  for i = 1:pad
      intArray[i] = parse(Int64, charArray[i])
  end

  return intArray
  =#

  # v2
  binaryString = bin(decimal, pad)
  charArray = collect(binaryString)

  # Option 1
  #intArray = map(x -> parse(Int64, x), charArray)#::Array{Int64}
  # error: #OptFramework failed to optimize function ##de2bi#7607 in optimization pass ParallelAccelerator.Driver.toParallelIR with error
  #UndefVarError(:x)

  # Option 2
  intArray = [parse(Int64, x) for x in charArray]::Array{Int64}
  # error: OptFramework failed to optimize function ##de2bi#11663 in optimization pass ParallelAccelerator.Driver.toParallelIR with error ErrorException("Conflicting variable x exists in both inner and outer lambda")

  intArrayReversed = reverse(intArray)
  intArrayReversedTransposed = transpose(intArrayReversed)
  return intArrayReversedTransposed
  #

end

1) When I enable Option 1 (with Option 2 disabled), I get an error:

OptFramework failed to optimize function ##de2bi#7607 in optimization pass ParallelAccelerator.Driver.toParallelIR with error UndefVarError(:x)

(According to DrTodd13's commment in Issue #23, it seems that map() cannot be parallelized.)

2) If I enable Option 2 (with Option 1 disabled), I get another error:

OptFramework failed to optimize function ##de2bi#11663 in optimization pass ParallelAccelerator.Driver.toParallelIR with error ErrorException("Conflicting variable x exists in both inner and outer lambda")

And I don't know what I should modify..

I would appreciate your comments.

ninegua commented 8 years ago

Thanks for reporting the issue. But before we discuss the details, I want to make sure that you understand the imposed Julia-to-C limitations would prevent ParallelAccelerator from running all Julia functions. A good way to find out what is supported is by looking at actual code samples in our test files.

Now for the above program, function call such as bin, collect, parse and reverse are not officially supported. For collect and parse, they both involve passing type values to a function's parameter, this is not supported by our CGen backend. For bin and reverse, although they are not tested before, they can be handled by CGen just fine (with latest commit bb26508ab8123bff951353189c5db16e6bfe2f83 where I added an intrinsic support required by bin).

Luckily, with a slight rewrite to avoid collect and parse, the following code works:

  intArray = Int64[convert(Int64, binaryString[x]) for x in 1:length(binaryString)]

But then again, the computation is kind of trivial, so I wouldn't expect any performance improvement.

Please let me know if this helps.

ninegua commented 8 years ago

Oh, by the way, map is actually supported for arrays of Number, but not for arrays of Char. It is unfortunate that Julia does not treat Char as a sub-type of Number.

Examples of map and reduce can be found in mapreduce.jl.

jihoonseo commented 8 years ago

Thank you for the detailed comments. They are so helpful.

jihoonseo commented 8 years ago

Sorry for asking basics, but I am not familiar with Julia package management and git.

1) How can I apply your commit bb26508? (I tried git checkout/pull on bash and Pkg.checkout/build on Julia but failed.)

2) After applying your commit, If I want to revert to the default status, how can I achieve this? (updated) -> I achieved this with Pkg.free("ParallelAccelerator"). Is this right?

3) In your suggested code, Int64[convert(Int64, binaryString[x])] changes 1(Char) to 49(Int64) and 0(Char) to 48(Int64), but I want to change 1(Char) to 1(Int64) and 0(Char) to 0(Int64). Do you have any idea? Or should I just subtract 48 from the converted result?

4) Although I did not applied your commit yet, I replaced collect and parse with your suggested code and got this error:

OptFramework failed to optimize function ##de2bi#12561 in optimization pass ParallelAccelerator.Driver.toCGen with error MethodError(ParallelAccelerator.CGen.toCtype,(:(x::UInt64),))

And I don't know why this occurs..

Again, I would appreciate your comments.

ninegua commented 8 years ago

To use master you can do as said in this documentation http://parallelacceleratorjl.readthedocs.io/en/latest/install.html

Let me know if you had any issue with it.

jihoonseo commented 8 years ago

Hi

I switched to master with these commands:

Pkg.checkout("ParallelAccelerator")     # Switch to master branch
Pkg.checkout("CompilerTools")           # Switch to master branch
Pkg.build("ParallelAccelerator")        # Build the C++ runtime component of the package.
Pkg.test("CompilerTools")               # Run CompilerTools tests.
Pkg.test("ParallelAccelerator")         # Run ParallelAccelerator tests.

and tested your suggestion with this code (replacing collect and parse with convert):

using ParallelAccelerator

@acc function de2bi(decimal, pad)
  binaryString = bin(decimal, pad)
  intArray = Int64[convert(Int64, binaryString[x])-48 for x in 1:length(binaryString)] # -48 : Char to Int64 offset
  intArrayReversed = reverse(intArray)
  intArrayReversedTransposed = transpose(intArrayReversed)
  return intArrayReversedTransposed
end

decimal = 10
pad = 4
result = de2bi(decimal, pad)
println(result)

and got a warning and a exceptional result.

1) Warning (It seems that this warning is related with the exceptional result following)

/tmp/tmpFEVIh2/cgen_output2.cpp: In function ‘ASCIIString _Base_bin(uint64_t, int64_t, bool)’: /tmp/tmpFEVIh2/cgen_output2.cpp:45:54: warning: right shift count >= width of type [-Wshift-count-overflow] GenSym2 = ((GenSym1) << (3)) - ( builtin_clz(x) >> 63 == 0 ? __builtin_clz(x) : builtin_clz(x)); ^ (<- In the Julia console, this hat is pointing the number 63)

1-1) In /tmp/tmpFEVIh2/cgen_output2.cpp:45(the line that the warning is pointing), there exists a function _Base_bin() that seems to be a C-converted version of Base.bin()

ASCIIString _Base_bin(uint64_t x, int64_t pad, bool neg)
{
        int64_t i;
        j2c_array< uint8_t >  a;
        char GenSym0;
        int64_t GenSym1;
        int64_t GenSym2;
        int64_t GenSym3;
        char GenSym4;
        char GenSym5;
        GenSym1 = sizeof(uint64_t);
        GenSym2 = ((GenSym1) << (3)) - ( __builtin_clz(x) >> 63 == 0 ?  __builtin_clz(x) :  __builtin_clz(x));
        i = ((int64_t) (neg)) + (((GenSym2) < (pad)) ? (pad) : (GenSym2));
        a = j2c_array<uint8_t>::new_j2c_array_1d(NULL, i);
        if (!(((int64_t) (neg)) < (i))) goto label1;
label2 : ;
         GenSym4 = '0';
         GenSym0 = (GenSym4 >> 31 == 0 ? GenSym4 : GenSym4) + ((int32_t)((x) & ((uint64_t) (1)) >> 63 == 0 ? (x) & ((uint64_t) (1)) : (x) & ((uint64_t) (1))));
         a.ARRAYELEM(i) = (uint8_t)(GenSym0);
         x = (x) >> (1);
         i = (i) - (1);
label3 : ;
         if (!(!(((int64_t) (neg)) < (i)))) goto label2;
label1 : ;
label0 : ;
         if (!(neg)) goto label4;
         GenSym5 = '-';
         a.ARRAYELEM(1) = (uint8_t)(GenSym5);
label4 : ;
         return ASCIIString{( j2c_array< uint8_t > )(a)};
}

2) Exceptional result : I called de2bi(10, 4) with the pad 4, but the result is completely wrong and seems to have the pad 64, not 4. This problem did not occur in 0.1.7.

[0 1 1 0 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1]

My guess is that some fix will be needed to CGen, or somewhere else..

Thank you.

ninegua commented 8 years ago

Thanks for reporting the bug. It was due to a wrong implementation of flipsign_int in CGen. I've fixed it as well as the warning you were getting. So now the correct result for de2bi(10, 4) is [0 1 0 1], and de2bi(10,10) is [0 1 0 1 0 0 0 0 0 0]. These matches native Julia result without going through ParallelAccelerator (if you run the same program with PROSPECT_MODE=none environment).

You'll need to update your package to get latest master for ParallelAccelerator.

jihoonseo commented 8 years ago

I updated the package and checked that it returns the correct result. Thank you.