This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] -ftree-loop-linear fixes (PR tree-optimization/46970) (take 2)
- From: Richard Guenther <richard dot guenther at gmail dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Sebastian Pop <sebpop at gmail dot com>, gcc-patches at gcc dot gnu dot org
- Date: Tue, 4 Jan 2011 17:22:42 +0100
- Subject: Re: [PATCH] -ftree-loop-linear fixes (PR tree-optimization/46970) (take 2)
- References: <20101217231141.GG2198@tyan-ft48-01.lab.bos.redhat.com> <AANLkTikcXwFf+oSuwfubFLtieu=Fg0cv49uZ3hgRqMcL@mail.gmail.com> <20101220222822.GB16156@tyan-ft48-01.lab.bos.redhat.com> <AANLkTimSYeJM3J1GnQFDnqopxrCaTnM-EgeME-ZixUOr@mail.gmail.com> <20110104124524.GH16156@tyan-ft48-01.lab.bos.redhat.com>
On Tue, Jan 4, 2011 at 1:45 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Dec 22, 2010 at 12:45:31PM +0100, Richard Guenther wrote:
>> > @@ -1332,7 +1332,9 @@ gcc_loop_to_lambda_loop (struct loop *lo
>> >
>> > ? ? ? return NULL;
>> > ? ? }
>> > - ?if (TREE_CODE (step) != INTEGER_CST)
>> > + ?if (!host_integerp (step, 0)
>> > + ? ? ?|| (HOST_WIDE_INT) TREE_INT_CST_LOW (step)
>> > + ? ? ? ? != (int) TREE_INT_CST_LOW (step))
>>
>> That casting looks odd. ?Why isn't the (supposed) use of step as 'int' not
>> changed to a HOST_WIDE_INT?
>
> I've briefly looked at it, but changing int to HOST_WIDE_INT would be a huge
> change, int is used basically everywhere in lambda-code, starting from
> *lambda_vector and lot of other places. ?Changing just LL_STEP to
> HOST_WIDE_INT wouldn't help. ?lambda-code looks very suspicious in many
> places, e.g. I don't see anywhere where it would bother with checking for
> overflows etc., wonder how it ever could get through patch review.
> Niceties like:
> ?switch (TREE_CODE (expr))
> ? ?{
> ? ?case INTEGER_CST:
> ? ? ?{
> ? ? ? ?lle = lambda_linear_expression_new (depth, 2 * depth, lambda_obstack);
> ? ? ? ?LLE_CONSTANT (lle) = TREE_INT_CST_LOW (expr);
> ? ? ? ?if (extra != 0)
> ? ? ? ? ?LLE_CONSTANT (lle) += extra;
>
> ? ? ? ?LLE_DENOMINATOR (lle) = 1;
> ? ? ?}
> ? ? ?break;
> where it both doesn't bother to check that expr is host_integerp and doesn't
> care if expr + extra doesn't overflow.
>
>> > @@ -1366,6 +1368,32 @@ gcc_loop_to_lambda_loop (struct loop *lo
>> > ? ? ? return NULL;
>> > ? ? }
>> >
>> > + ?if (SSA_NAME_DEF_STMT (inductionvar) != phi)
>> > + ? ?{
>> > + ? ? ?gimple stmt = SSA_NAME_DEF_STMT (inductionvar);
>> > + ? ? ?tree rhs2;
>> > + ? ? ?int stepintval = stepint;
>> > + ? ? ?switch (gimple_assign_rhs_code (stmt))
>> > + ? ? ? {
>> > + ? ? ? case MINUS_EXPR:
>> > + ? ? ? ? stepintval = -stepint;
>> > + ? ? ? ? /* FALLTHRU */
>> > + ? ? ? case PLUS_EXPR:
>> > + ? ? ? case POINTER_PLUS_EXPR:
>> > + ? ? ? ? rhs2 = gimple_assign_rhs2 (stmt);
>> > + ? ? ? ? if (TREE_CODE (rhs2) == INTEGER_CST
>> > + ? ? ? ? ? ? && (HOST_WIDE_INT) TREE_INT_CST_LOW (rhs2) == stepintval
>> > + ? ? ? ? ? ? && TREE_INT_CST_HIGH (rhs2) == (stepintval >= 0 ? 0 : -1))
>>
>> That would also simplify this code - why not tree_int_cst_equal (step,
>> rhs2) btw?
>
> Because for MINUS_EXPR we don't want to check step, but - step ?
> And compare_tree_int is an UHWI comparison instead of HWI.
Ugh. Sebastian - can we nuke tree-loop-linear compeltely and
make -ftree-loop-linear an alias for -floop-interchange without
regressions? I'd like to reduce the number of broken passes from
2 to 1 this way ...
Richard.
> ? ? ? ?Jakub
>