| Message ID | CABGF_gdStNVACNpMiOMm1U-HWVv=M2iFE=g4ejF16z99nxO3DA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 30/07/2014 09:56, Roman Gareev wrote: >> Blindly converting type sizes is not a good idea and may be problematic when >> >we switch to smaller types. As we can obviously only improve this when we >> >have the appropriate support in isl, I think the attached patch is fine. >> >However, please add a fixme that states that this should >> >be looked at again at the moment when we get isl support to derive the >> >optimal types. > I've attached a patch, which contains the fixme. OK. I proposed a slightly longer description. With the updated description, you can commit this patch. >> >Please have a look at the original bug report: >> > >> >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=35356 >> > >> >The isl ast generator should produce something like: >> > >> > for (i = 0; i < min (n, k); i++) >> > a[i] = i; >> > if (k >= 0 && k < n) >> > a[k] = 1; >> > for (i = max(k+1,0); i < n; i++) >> > a[i] = i; >> > >> >CLooG does not generate optimal code either, but the code generated by isl >> >is a regression compared to CLooG. >> > >> >Can you generate an isl_codegen input for this test case and verify that the >> >latest public version of isl does not generate this optimal code either. If >> >it does not, please report this as an optimization bug to the isl mailing >> >list. >> > >> >After you got feedback, I think it is OK to disable this test and to >> >add a FIXME explaining why this test is disabled and if we can expect a fix >> >in later versions of isl. > I've generated the code using the isl-0.13 > > if (n >= k + 1) { > for (int c1 = 0; c1 < n; c1 += 1) { > if (c1 >= k + 1) { > S_7(c1); > } else if (k >= c1 + 1) { > S_7(c1); > } else > S_6(k); > } > } else > for (int c1 = 0; c1 < n; c1 += 1) > S_7(c1); > > If I'm not mistaken it's also a regression compared to ClooG and it's > better to report this to the isl mailing list. That's what I suggested earlier, no? The only thing I would like you to check beforehand is if the very same problem still exists with isl trunk. For this you need to generate an input file for the 'isl_codegen' tool as it comes with isl that allows to reproduce the bug. The very same input file can then be submitted to the isl mailing list as a bug report. Could you do the reporting (and CC me)? >> >What is the problem with this last test case? > It is related to checking for the loop parallelism, which is not yet > implemented in the current version of graphite-isl-ast-to-gimple.c. OK. So yes, you are right that we need the parallelism test soon. Cheers, Tobias
> OK. I proposed a slightly longer description. With the updated description, > you can commit this patch. I've fixed the fixme. FIXME: We should replace blind conversation of id's type with derivation of the optimal type when we get the corresponding isl support. Blindly converting type sizes may be problematic when we switch to smaller types. Would you add anything to it? > That's what I suggested earlier, no? The only thing I would like you to > check beforehand is if the very same problem still exists with isl trunk. > For this you need to generate an input file for the 'isl_codegen' tool as it > comes with isl that allows to reproduce the bug. The very same input file > can then be submitted to the isl mailing list as a bug report. Could you do > the reporting (and CC me)? Yes, I wanted to make sure that the code generated by the isl-0.13 is also not optimal (I sent you the code generated by isl-0.12.2 before). The report has been sent to the isl mailing list and CC you. -- Cheers, Roman Gareev.
On 30/07/2014 14:32, Roman Gareev wrote: >> OK. I proposed a slightly longer description. With the updated description, >> you can commit this patch. > > I've fixed the fixme. > > FIXME: We should replace blind conversation of id's type with > derivation of the optimal type when we get the corresponding isl > support. Blindly converting type sizes may be problematic when we > switch to smaller types. > Would you add anything to it? Fine with me. >> That's what I suggested earlier, no? The only thing I would like you to >> check beforehand is if the very same problem still exists with isl trunk. >> For this you need to generate an input file for the 'isl_codegen' tool as it >> comes with isl that allows to reproduce the bug. The very same input file >> can then be submitted to the isl mailing list as a bug report. Could you do >> the reporting (and CC me)? > > Yes, I wanted to make sure that the code generated by the isl-0.13 is > also not optimal (I sent you the code generated by isl-0.12.2 before). > The report has been sent to the isl mailing list and CC you. Great. Thanks. (Sven already replied) Two comments on your bug report: 1) It would be helpful to also point give the output CLooG generates for similar input 2) The input is a little involved. If you can provide a minimal test case, this normally helps in getting bugs fixed faster. Cheers, Tobias
Could you please advise me how is it better to answer the following questions of Sven: > In what way is it "not optimal"? > That is, what are your optimality criteria? (I could answer them, but I don't want to miss anything) -- Cheers, Roman Gareev.
On 31/07/2014 08:19, Roman Gareev wrote: > Could you please advise me how is it better to answer the following > questions of Sven: > >> In what way is it "not optimal"? >> That is, what are your optimality criteria? > > (I could answer them, but I don't want to miss anything) Don't worry. Just give it a shot. ;-) (I even don't understand the question in this case. Not having conditions in the loops reduces control overhead, while in this case it does not seem to come with code size growth either.) Tobias
Index: gcc/graphite-isl-ast-to-gimple.c =================================================================== --- gcc/graphite-isl-ast-to-gimple.c (revision 213109) +++ gcc/graphite-isl-ast-to-gimple.c (working copy) @@ -122,10 +122,14 @@ ivs_params &ip); /* Return the tree variable that corresponds to the given isl ast identifier - expression (an isl_ast_expr of type isl_ast_expr_id). */ + expression (an isl_ast_expr of type isl_ast_expr_id). + FIXME: We should replace blind conversation of id's type with derivation + of the optimal type when we get the corresponding isl support. */ + static tree -gcc_expression_from_isl_ast_expr_id (__isl_keep isl_ast_expr *expr_id, +gcc_expression_from_isl_ast_expr_id (tree type, + __isl_keep isl_ast_expr *expr_id, ivs_params &ip) { gcc_assert (isl_ast_expr_get_type (expr_id) == isl_ast_expr_id); @@ -136,7 +140,7 @@ gcc_assert (res != ip.end () && "Could not map isl_id to tree expression"); isl_ast_expr_free (expr_id); - return res->second; + return fold_convert (type, res->second); } /* Converts an isl_ast_expr_int expression E to a GCC expression tree of @@ -351,7 +355,7 @@ switch (isl_ast_expr_get_type (expr)) { case isl_ast_expr_id: - return gcc_expression_from_isl_ast_expr_id (expr, ip); + return gcc_expression_from_isl_ast_expr_id (type, expr, ip); case isl_ast_expr_int: return gcc_expression_from_isl_expr_int (type, expr);