| Message ID | CABGF_gfCmbvkSmwbzH20DibgUQvCj7WFasQw1yaRSOoNTPR6hA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 08/08/2014 15:11, Roman Gareev wrote: >> I think this is fine. On the other side, I think the test case itself >> >is unnecessarily large. I would assume the majority of the code could >> >be deleted while still triggering the bug. Could you give it a shot. > I've attached an improved version of the patch. LGTM. >> >Is there anything else you still would like to do? > I wanted to make the current code available to be able to fix all new > possible bugs before 'pencils down'. If I have free time, I'll try to > improve the performance of the generation. Good, though before let's try to get the correctness right. >> >Otherwise, I believe fixing the last remaining bugs is of high importance, as we want to enable >> >this code as soon as possible to avoid future bitrot. >> > >> >I understand that reducing this may require some work, but I don't think it >> >is that hard. Specifically, you could first compile the individual *.cpp >> >files to .o files once using once graphite once not. > I've found out that btCollisionWorld.cpp, btCollisionDispatcher.cpp > and btDiscreteDynamicsWorld.llvm.cpp cause “Segmentation fault”. All > of them produce similar ASTs: Is this segmentation fault at compile time or at run-time? I believe it was a segfault at run-time due to a miscompile. > btCollisionWorld.cpp: > > CLAST generated by CLooG: > > if (8*T_45 >= -T_44+9) { > for (scat_1=0;scat_1<=T_45-1;scat_1++) { > if ((8*scat_1+T_44+18446744073709551615)%18446744073709551616 <= > 18446744073709551614) { > (scat_1); > } > } > } > > isl_codegen: > > [P_45, P_44] -> { S_12[i0] -> [0, i0, 0] : exists (e0 = floor((-1 + > P_45)/4294967296), e1 = floor((P_44 + 8i0)/18446744073709551616): i0 >> >= 0 and 4294967296e0 <= -1 + P_45 and 4294967296e0 >= -4294967296 + > P_45 and 4294967296e0 <= -1 + P_45 - i0 and i0 <= 2147483646 and > 18446744073709551616e1 >= -18446744073709551615 + P_44 + 8i0 and > 18446744073709551616e1 <= -1 + P_44 + 8i0) } > > [P_45, P_44] -> { : exists (e0 = floor((-1 + P_45)/4294967296): > 4294967296e0 <= -1 + P_45 and 4294967296e0 >= -2147483647 + P_45 and > P_45 >= -2147483648 and P_45 <= 2147483647 and P_44 >= 0 and P_44 <= > 18446744073709551615) } > > [P_45, P_44] -> { [i0, i1, i2] -> separate[o0] } > > ISL AST generated by ISL: > > for (int c1 = 0; c1 < P_45; c1 += 1) > if ((P_44 + 8 * c1) % 18446744073709551616 >= 1) > S_12(c1); > > btCollisionDispatcher.cpp: > > CLAST generated by CLooG: > > if (8*T_81 >= -T_80+9) { > for (scat_1=0;scat_1<=T_81-1;scat_1++) { > if ((8*scat_1+T_80+18446744073709551615)%18446744073709551616 <= > 18446744073709551614) { > (scat_1); > } > } > } > > isl_codegen: > > [P_81, P_80] -> { S_22[i0] -> [0, i0, 0] : exists (e0 = floor((-1 + > P_81)/4294967296), e1 = floor((P_80 + 8i0)/18446744073709551616): i0 >> >= 0 and 4294967296e0 <= -1 + P_81 and 4294967296e0 >= -4294967296 + > P_81 and 4294967296e0 <= -1 + P_81 - i0 and i0 <= 2147483646 and > 18446744073709551616e1 >= -18446744073709551615 + P_80 + 8i0 and > 18446744073709551616e1 <= -1 + P_80 + 8i0) } > > [P_81, P_80] -> { : exists (e0 = floor((-1 + P_81)/4294967296): > 4294967296e0 <= -1 + P_81 and 4294967296e0 >= -2147483647 + P_81 and > P_81 >= -2147483648 and P_81 <= 2147483647 and P_80 >= 0 and P_80 <= > 18446744073709551615) } > > [P_81, P_80] -> { [i0, i1, i2] -> separate[o0] } > > ISL AST generated by ISL: > > for (int c1 = 0; c1 < P_81; c1 += 1) > if ((P_80 + 8 * c1) % 18446744073709551616 >= 1) > S_22(c1); > > btDiscreteDynamicsWorld.llvm.cpp: > > CLAST generated by CLooG: > > if (8*T_24 >= -T_23+9) { > for (scat_1=0;scat_1<=T_24-1;scat_1++) { > if ((8*scat_1+T_23+18446744073709551615)%18446744073709551616 <= > 18446744073709551614) { > (scat_1); > } > } > } > > isl_codegen: > > [P_24, P_23] -> { S_13[i0] -> [0, i0, 0] : exists (e0 = floor((-1 + > P_24)/4294967296), e1 = floor((P_23 + 8i0)/18446744073709551616): i0 >> >= 0 and 4294967296e0 <= -1 + P_24 and 4294967296e0 >= -4294967296 + > P_24 and 4294967296e0 <= -1 + P_24 - i0 and i0 <= 2147483646 and > 18446744073709551616e1 >= -18446744073709551615 + P_23 + 8i0 and > 18446744073709551616e1 <= -1 + P_23 + 8i0) } > > [P_24, P_23] -> { : exists (e0 = floor((-1 + P_24)/4294967296): > 4294967296e0 <= -1 + P_24 and 4294967296e0 >= -2147483647 + P_24 and > P_24 >= -2147483648 and P_24 <= 2147483647 and P_23 >= 0 and P_23 <= > 18446744073709551615) } > > [P_24, P_23] -> { [i0, i1, i2] -> separate[o0] } > > ISL AST generated by ISL: > > for (int c1 = 0; c1 < P_24; c1 += 1) > if ((P_23 + 8 * c1) % 18446744073709551616 >= 1) > S_13(c1); > > CLAST generated by CLooG: > > if (8*T_46 >= -T_45+9) { > for (scat_1=0;scat_1<=T_46-1;scat_1++) { > if ((8*scat_1+T_45+18446744073709551615)%18446744073709551616 <= > 18446744073709551614) { > (scat_1); > } > } > } > > isl_codegen: > > [P_46, P_45] -> { S_16[i0] -> [0, i0, 0] : exists (e0 = floor((-1 + > P_46)/4294967296), e1 = floor((P_45 + 8i0)/18446744073709551616): i0 >> >= 0 and 4294967296e0 <= -1 + P_46 and 4294967296e0 >= -4294967296 + > P_46 and 4294967296e0 <= -1 + P_46 - i0 and i0 <= 2147483646 and > 18446744073709551616e1 >= -18446744073709551615 + P_45 + 8i0 and > 18446744073709551616e1 <= -1 + P_45 + 8i0) } > > [P_46, P_45] -> { : exists (e0 = floor((-1 + P_46)/4294967296): > 4294967296e0 <= -1 + P_46 and 4294967296e0 >= -2147483647 + P_46 and > P_46 >= -2147483648 and P_46 <= 2147483647 and P_45 >= 0 and P_45 <= > 18446744073709551615) } > > [P_46, P_45] -> { [i0, i1, i2] -> separate[o0] } > > ISL AST generated by ISL: > > for (int c1 = 0; c1 < P_46; c1 += 1) > if ((P_45 + 8 * c1) % 18446744073709551616 >= 1) > S_16(c1); > > CLAST generated by CLooG: > > if (8*T_18 >= -T_17+9) { > for (scat_1=0;scat_1<=T_18-1;scat_1++) { > if ((8*scat_1+T_17+18446744073709551615)%18446744073709551616 <= > 18446744073709551614) { > (scat_1); > } > } > } > > isl_codegen: > > [P_18, P_17] -> { S_12[i0] -> [0, i0, 0] : exists (e0 = floor((-1 + > P_18)/4294967296), e1 = floor((P_17 + 8i0)/18446744073709551616): i0 >> >= 0 and 4294967296e0 <= -1 + P_18 and 4294967296e0 >= -4294967296 + > P_18 and 4294967296e0 <= -1 + P_18 - i0 and i0 <= 2147483646 and > 18446744073709551616e1 >= -18446744073709551615 + P_17 + 8i0 and > 18446744073709551616e1 <= -1 + P_17 + 8i0) } > > [P_18, P_17] -> { : exists (e0 = floor((-1 + P_18)/4294967296): > 4294967296e0 <= -1 + P_18 and 4294967296e0 >= -2147483647 + P_18 and > P_18 >= -2147483648 and P_18 <= 2147483647 and P_17 >= 0 and P_17 <= > 18446744073709551615) } > > [P_18, P_17] -> { [i0, i1, i2] -> separate[o0] } > > ISL AST generated by ISL: > > for (int c1 = 0; c1 < P_18; c1 += 1) > if ((P_17 + 8 * c1) % 18446744073709551616 >= 1) > S_12(c1); > > I think that, for example, the following code > > if (8*T_45 >= -T_44+9) { > for (scat_1=0;scat_1<=T_45-1;scat_1++) { > if ((8*scat_1+T_44+18446744073709551615)%18446744073709551616 <= > 18446744073709551614) { > (scat_1); > } > } > } > > is equivalent to > > if (8*T_45 >= -T_44+9) { > for (scat_1=0;scat_1<=T_45-1;scat_1++) { > if ((8*scat_1+T_44)%18446744073709551616 <= -1) { > (scat_1); > } > } > } > > If I'm not mistaken it's not equivalent to > > for (int c1 = 0; c1 < P_45; c1 += 1) > if ((P_44 + 8 * c1) % 18446744073709551616 >= 1) > S_12(c1); > > Maybe the ISL generator generates wrong code. What do you think about this? Possibly. Can you split the .cpp files such that you only compile a single function with graphite and that compiling this function causes the miscompile. This allows us to look at less code. Cheers, Tobias
> Is this segmentation fault at compile time or at run-time? I believe it was > a segfault at run-time due to a miscompile. Yes, it's a segfault at run-time. These source codes produce wrong object files. > Possibly. Can you split the .cpp files such that you only compile a single > function with graphite and that compiling this function causes the > miscompile. This allows us to look at less code. I've tried to reduce btCollisionWorld.cpp and btCollisionDispatcher.cpp (They can be found attached). Should we ask Sven?
On 09/08/2014 09:42, Roman Gareev wrote: >> Is this segmentation fault at compile time or at run-time? I believe it was >> a segfault at run-time due to a miscompile. > > Yes, it's a segfault at run-time. These source codes produce wrong object files. > >> Possibly. Can you split the .cpp files such that you only compile a single >> function with graphite and that compiling this function causes the >> miscompile. This allows us to look at less code. > > I've tried to reduce btCollisionWorld.cpp and > btCollisionDispatcher.cpp (They can be found attached). Should we ask > Sven? With just C++ code, Sven can help little. He has no knowledge about graphite internals. Do you now have a setup where you can just compile one of the attached files with graphite and everything else without and the code crashes? Why did you provide two files. I assume one should be sufficient to reproduce the crash, no? I think for the one file you choose, it would be interesting to look at the code generation input as well as the kernels generated by CLooG and isl. Maybe we can understand what is going on. Cheers, Tobias
> With just C++ code, Sven can help little. He has no knowledge about graphite > internals. I want to ask him about correctness of ISL ASTs generated from mentioned isl_codegens. > Do you now have a setup where you can just compile one of the attached files > with graphite and everything else without and the code crashes? No, I don't. As I mentioned earlier, all the files from this test case individually produce object files which linked into the one executable, which cause “Segmentation fault (core dumped)” (It'll cause “Segmentation fault (core dumped)”, if we try to run it). I think that it's very difficult to detach the code, which would produce the executable and save the semantics of this big OOP project. Furthermore, there is a possibility that the detached code can produce no new useful information. I've found that if we try to compile any of the btCollisionWorld.cpp, btCollisionDispatcher.cpp and btDiscreteDynamicsWorld.llvm.cpp by modified Graphite, the produced object file will lead to creation of the wrong executable (after linking with other object files generated by origin Graphite). That's why I think that we could consider only, for example, btCollisionDispatcher.cpp. It contains only one scope, which is incorrectly processed by Graphite with the ISL code generator as the main generator. > Why did you provide two files. I assume one should be sufficient to > reproduce the crash, no? I've detached the code, which is used by Graphite to produce similar AST's. > I think for the one file you choose, it would be interesting to look > at the code generation input as well as the kernels generated by CLooG and > isl. Maybe we can understand what is going on. What do you mean by the kernels generated by CLooG and isl? btCollisionDispatcher.cpp: isl_codegen: [P_2, P_11] -> { S_6[i0] -> [0, i0, 0] : exists (e0 = floor((-1 + P_2)/4294967296), e1 = floor((P_11 + 8i0)/18446744073709551616): i0 >= 0 and 4294967296e0 <= -1 + P_2 and 4294967296e0 >= -4294967296 + P_2 and 4294967296e0 <= -1 + P_2 - i0 and i0 <= 2147483646 and 18446744073709551616e1 >= -18446744073709551615 + P_11 + 8i0 and 18446744073709551616e1 <= -1 + P_11 + 8i0) } [P_2, P_11] -> { : exists (e0 = floor((-1 + P_2)/4294967296): 4294967296e0 <= -1 + P_2 and 4294967296e0 >= -2147483647 + P_2 and P_2 >= -2147483648 and P_2 <= 2147483647 and P_11 >= 0 and P_11 <= 18446744073709551615) } [P_2, P_11] -> { [i0, i1, i2] -> separate[o0] } ISL AST generated by ISL: for (int c1 = 0; c1 < P_2; c1 += 1) if ((P_11 + 8 * c1) % 18446744073709551616 >= 1) S_6(c1); cloog_input: # CLooG -> CLooG # This is an automatic dump of a CLooG input file from a CloogInput data # structure. # Language: C c # Context: 1 4 4 0 0 0 2 1 1 0 -1 1 -1 0 2147483647 1 0 -1 18446744073709551615 1 0 1 0 1 # Parameter name(s) T_2 T_11 # Statement number: 1 # Iteration domain of statement 1 (). 1 7 7 1 0 2 2 1 1 0 0 0 0 0 1 0 -4294967296 0 1 0 -1 1 0 4294967296 0 -1 0 4294967296 1 -1 -4294967296 0 1 0 -1 1 -1 0 0 0 0 2147483646 1 -8 0 18446744073709551616 0 -1 18446744073709551615 1 8 0 -18446744073709551616 0 1 -1 0 0 0 # For future options. 1 # Iterator name(s) git_0 # --------------------- SCATTERING -------------------- 1 # Scattering functions # Scattering of statement 1 (). 1 3 8 3 1 0 2 0 0 0 1 0 0 0 0 0 0 1 0 -1 0 0 0 0 1 0 0 0 0 0 0 1 # Scattering dimension name(s) scat_0 scat_1 scat_2 CLAST generated by ClooG: if (8*T_2 >= -T_11+9) { for (scat_1=0;scat_1<=T_2-1;scat_1++) { if ((8*scat_1+T_11+18446744073709551615)%18446744073709551616 <= 18446744073709551614) { (scat_1); } } }
On 09/08/2014 12:05, Roman Gareev wrote: >> With just C++ code, Sven can help little. He has no knowledge about graphite >> internals. > > I want to ask him about correctness of ISL ASTs generated from > mentioned isl_codegens. I am not sure if he can see it just like this. >> Do you now have a setup where you can just compile one of the attached files >> with graphite and everything else without and the code crashes? > > No, I don't. As I mentioned earlier, all the files from this test case > individually produce object files which linked into the one > executable, which cause “Segmentation fault (core dumped)” (It'll > cause “Segmentation fault (core dumped)”, if we try to run it). I > think that it's very difficult to detach the code, which would produce > the executable and save the semantics of this big OOP project. > Furthermore, there is a possibility that the detached code can produce > no new useful information. > > I've found that if we try to compile any of the btCollisionWorld.cpp, > btCollisionDispatcher.cpp > and btDiscreteDynamicsWorld.llvm.cpp by modified Graphite, the > produced object file will lead to creation of the wrong executable > (after linking with other object files generated by origin Graphite). I am confused. It seems you attached some kind of reduced version of btCollisionWorld.cpp? How did you obtain it? Did you compile and test with these versions? > That's why I think that we could consider only, for example, > btCollisionDispatcher.cpp. It contains only one scope, which is > incorrectly processed by Graphite with the ISL code generator as the > main generator. > >> Why did you provide two files. I assume one should be sufficient to >> reproduce the crash, no? > > I've detached the code, which is used by Graphite to produce similar AST's. OK. But did you check if they actually segfault or fail? >> I think for the one file you choose, it would be interesting to look >> at the code generation input as well as the kernels generated by CLooG and >> isl. Maybe we can understand what is going on. Yes, exactly those that you provided. At a first look, I don't see anything obvious wrong, except that the huge modulo values, which arise from some integer wrapping support Sebastian added once for unsigned integers. I see two approaches you can take: 1) The easy one Disallow unsigned integers in the scop detection. I have doubts we can do anything good with those huge modulo constraints placed all over the generated code 2) Try to understand the test case I would try a couple of easy unsigned int loops to see if we can handle them properly. Something like: for (unsigned long i = 0; i < M; i++) A[i] += i; At best, we could even write your test case as such a loop and reproduce the bug outside of this huge C++ code. Maybe looking at the GIMPLE allows you to write C code that has similar behavior and where we could check the output? Tobias
Index: gcc/graphite-isl-ast-to-gimple.c =================================================================== --- gcc/graphite-isl-ast-to-gimple.c (revision 213690) +++ gcc/graphite-isl-ast-to-gimple.c (working copy) @@ -632,9 +632,9 @@ gcc_assert (GBB_BB (gbb) != ENTRY_BLOCK_PTR_FOR_FN (cfun) && "The entry block should not even appear within a scop"); - loop_p loop = gbb_loop (gbb); - iv_map.create (loop->num + 1); - iv_map.safe_grow_cleared (loop->num + 1); + int nb_loops = number_of_loops (cfun); + iv_map.create (nb_loops); + iv_map.safe_grow_cleared (nb_loops); build_iv_mapping (iv_map, gbb, user_expr, ip, SCOP_REGION (pbb->scop)); isl_ast_expr_free (user_expr); Index: gcc/testsuite/gcc.dg/graphite/isl-ast-gen-user-1.c =================================================================== --- gcc/testsuite/gcc.dg/graphite/isl-ast-gen-user-1.c (revision 0) +++ gcc/testsuite/gcc.dg/graphite/isl-ast-gen-user-1.c (working copy) @@ -0,0 +1,25 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fgraphite-identity -fgraphite-code-generator=isl" } */ + +#include <stdio.h> +#include <stdlib.h> + +static const int N = 12; + +void Crystal_Cholesky (int nSlip, int a[N][N]) +{ + int i, j, k, fdot = 0; + + for ( i = 1; i < nSlip; i++) + { + for ( j = i+1; j < nSlip; j++) + { + for ( k = 0; k < i; k++) + fdot += a[i][k] * a[k][j]; + a[i][j] = a[i][j] - fdot; + } + } +} + + +