On Sun, Jul 17, 2011 at 19:41, Tobias Grosser <tob...@grosser.es> wrote:
>> Accessing a[100] is undefined in C, so i< 100, and so n<= 100.
>
> Sure. I am just surprised we already include this information in graphite.
> Do we add this to the context?
Yes, that's added to the context as a bound on the loop iteration domain.
> I just looked again through the gcc_type_ ... types and wondered why it
> contains both calls to max_precision_type() and max_signed_precision_type().
> Is there a reason for this?
Well, it's all the same: max_precision_type calls
max_signed_precision_type when one of the types is signed.
Also we are calling max_signed_precision_type to determine the type of
the induction variable, as we want to generate signed IV types as much
as possible.
> I also wondered if the solution of reusing the gcc_type_for_clast functions
> is the right one. As far as I understood the reason we do not use the type
> calculated by the lb_ub functions is that it may be too small and that it
> may introduce too many casts. So where exactly do we introduce these casts?
We introduce casts when we generate code for an expression with a type
smaller than the types of its constituents.
Here is an example: supposing that we already code generated an IV
graphite_IV with a signed short type, then we are asked to generate
code for the expression "42 * graphite_IV", and finally suppose that
lb_ub returns the interval [-100, 100] for this expression, then we
would have a signed char type for the expression, and so we would
introduce a cast of the graphite_IV to signed char before doing the
multiplication: so I guess the code generated for this expression
would be something like: "42 * (char) graphite_IV" of type char.
Without taking the max of the types of the sub-expressions, you would
have to generate casts for a shorter type.
> Can we find a solution that solves this problem more locally. I mean instead
> of using an imprecise algorithm that leads to larger types which
> (accidentally?) leads to less casts, we could understand the type calculated
> by lb_ub_... as a minimal type needed for correctness. For each instruction
> that we code generate, we derive the type by electing a type that is large
> enough to be correct and that at the same time is not smaller than the
> smallest type of each operand.
>
You surely mean "not smaller than the *largest* type of each operand".
Yes, that's the intention of the gcc_type_for_clast_* functions, get a
max over the types of sub-expressions.
> If CLoog one day provides the lb/ub information itself, the migration pass
> will be straightforward.
>
> What do you think?
I agree with you that CLooG should provide this information for both
induction variables, and for each expression of the CLAST. We should
not duplicate code doing the same thing in different compilers that
use CLooG.
>> +static struct clast_user_stmt *
>> +clast_get_body_of (struct clast_stmt *stmt)
>> +{
>> + if (!stmt
>> + || CLAST_STMT_IS_A (stmt, stmt_user))
>> + return (struct clast_user_stmt *) stmt;
>> +
>> + if (CLAST_STMT_IS_A (stmt, stmt_for))
>> + return clast_get_body_of (((struct clast_for *) stmt)->body);
>> +
>> + if (CLAST_STMT_IS_A (stmt, stmt_guard))
>> + return clast_get_body_of (((struct clast_guard *) stmt)->then);
>> +
>> + if (CLAST_STMT_IS_A (stmt, stmt_block))
>> + return clast_get_body_of (((struct clast_block *) stmt)->body);
>> +
>> + gcc_unreachable ();
>> +}
>
> The use of this function seems incorrect to me. We seem to only get the
> first statement in a loop and use its domain and scattering to derive
> the size of the iv type. Let's assume we have this code:
>
> for (s0 = 0; s0 <= n; s0++) {
> if (s0 = 0)
> S1(s0);
> S2(s0);
> }
>
> Here we would get a range [0,0] instead of [0,n]. This does not seem to
> be correct. A solution would be to completely switch to cloog-isl and
> just use the information that the clast provides about the enumerated
> space in each loop.
>
You're right. However this cleanup should be in a different patch,
probably after another patch set that removes support for CLooG-ppl.
Sebastian
