On 26/07/2014 11:53, Roman Gareev wrote:
Any reason you don't use isl_id_for_pbb() to create this isl_id?
Yes, it is redundant. I've used isl_id_for_pbb() in the improved version.
Otherwise, the patch looks good to me. You can commit it if the graphite tests
still pass and you add an appropriate ChangeLog entry.
I haven't found out regression during testing with the graphite tests.
This patch looks also good. Can you quickly repost with the two test cases as
well as the appropriate ChangeLog, before I give the final OK.
If I'm not mistaken, I sent you only one test case. Should we create
another one?
Right, I got confused.
I would still add a test case which does not contain a reduction (+=)
and where graphite is not duplicating pbbs.
If you make res of type float I assume graphite will not detect it as a
reduction. On the other side, it may not even introduce if conditions.
So you may need a little bit more complicated code e.g:
for (i = 0; i < 50; i++)
{
res += i * 2.1;
if (i >= 25)
res += i * 3;
}
This should in the best case yield an isl_ast which matches the input code.
Also, please add a comment to the other test case that notes what kind
of issue this one is testing (reduction, where the pbbs are possibly
duplicated).
Cheers,
Tobias
