Patchwork [fortran,1/5] PR 45648: Inline transpose part 2: Support transposed descriptor whilst getting array bounds

login
register
mail settings
Submitter Mikael Morin
Date Sept. 20, 2010, 10:21 p.m.
Message ID <20100920222130.27269.19670@gimli.local>
Download mbox | patch
Permalink /patch/65260/
State New
Headers show

Comments

Mikael Morin - Sept. 20, 2010, 10:21 p.m.
We can't have dim (new descriptor's dimension) increasing together with n (original array's dimension) if we want to support transposed reference. We have to get dim such that info->dim[dim] == n. Then we can set the descriptor bounds along dim.

OK for trunk?
2010-09-20  Mikael Morin  <mikael@gcc.gnu.org>

	PR fortran/45648
	* trans-array.c (gfc_conv_expr_descriptor): Calculate dim out of n and
	info->dim.
Paul Richard Thomas - Sept. 21, 2010, 1:47 p.m.
Mikael,

This is OK except that I do not understand what the gcc_assert is doing in:
+
+	  /* look for the corresponding scalarizer dimension: dim.  */
+	  for (dim = 0; dim < ndim; dim++)
+	    if (info->dim[dim] == n)
+	      break;
+	  gcc_assert (dim < ndim);

Have I missed something?

Cheers

Paul

On 9/21/10, Mikael Morin <mikael.morin@sfr.fr> wrote:
> We can't have dim (new descriptor's dimension) increasing together with n
> (original array's dimension) if we want to support transposed reference. We
> have to get dim such that info->dim[dim] == n. Then we can set the
> descriptor bounds along dim.
>
> OK for trunk?
>
>
>
Daniel Kraft - Sept. 21, 2010, 2:13 p.m.
Paul Richard Thomas wrote:
> Mikael,
> 
> This is OK except that I do not understand what the gcc_assert is doing in:
> +
> +	  /* look for the corresponding scalarizer dimension: dim.  */
> +	  for (dim = 0; dim < ndim; dim++)
> +	    if (info->dim[dim] == n)
> +	      break;
> +	  gcc_assert (dim < ndim);
> 
> Have I missed something?

This seems like checking the loop is exited early in any case.  (But 
I've no idea about the rest of the patch.)

Daniel
Paul Richard Thomas - Sept. 21, 2010, 2:46 p.m.
Dominique,

The loop takes dim from 0 to ndim-1.  Therefore the condition of the
gcc_assert is always going to be satisfied.

Cheers

Paul

On 9/21/10, Daniel Kraft <d@domob.eu> wrote:
> Paul Richard Thomas wrote:
>> Mikael,
>>
>> This is OK except that I do not understand what the gcc_assert is doing
>> in:
>> +
>> +	  /* look for the corresponding scalarizer dimension: dim.  */
>> +	  for (dim = 0; dim < ndim; dim++)
>> +	    if (info->dim[dim] == n)
>> +	      break;
>> +	  gcc_assert (dim < ndim);
>>
>> Have I missed something?
>
> This seems like checking the loop is exited early in any case.  (But
> I've no idea about the rest of the patch.)
>
> Daniel
>
> --
> http://www.pro-vegan.info/
> --
> Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
> To go: Hea-Mon-Pri
>
Paul Richard Thomas - Sept. 21, 2010, 2:47 p.m.
s/Dominique/Daniel/ :-)

Paul

On 9/21/10, Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
> Dominique,
>
> The loop takes dim from 0 to ndim-1.  Therefore the condition of the
> gcc_assert is always going to be satisfied.
>
> Cheers
>
> Paul
>
> On 9/21/10, Daniel Kraft <d@domob.eu> wrote:
>> Paul Richard Thomas wrote:
>>> Mikael,
>>>
>>> This is OK except that I do not understand what the gcc_assert is doing
>>> in:
>>> +
>>> +	  /* look for the corresponding scalarizer dimension: dim.  */
>>> +	  for (dim = 0; dim < ndim; dim++)
>>> +	    if (info->dim[dim] == n)
>>> +	      break;
>>> +	  gcc_assert (dim < ndim);
>>>
>>> Have I missed something?
>>
>> This seems like checking the loop is exited early in any case.  (But
>> I've no idea about the rest of the patch.)
>>
>> Daniel
>>
>> --
>> http://www.pro-vegan.info/
>> --
>> Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
>> To go: Hea-Mon-Pri
>>
>
>
> --
> The knack of flying is learning how to throw yourself at the ground and
> miss.
>        --Hitchhikers Guide to the Galaxy
>
Richard Guenther - Sept. 21, 2010, 2:49 p.m.
On Tue, Sep 21, 2010 at 4:46 PM, Paul Richard Thomas
<paul.richard.thomas@gmail.com> wrote:
> Dominique,
>
> The loop takes dim from 0 to ndim-1.  Therefore the condition of the
> gcc_assert is always going to be satisfied.

But if the loop terminates via its exit condition !(dim<ndim) then
obviously dim will be not < ndim ;)

RIchard.

> Cheers
>
> Paul
>
> On 9/21/10, Daniel Kraft <d@domob.eu> wrote:
>> Paul Richard Thomas wrote:
>>> Mikael,
>>>
>>> This is OK except that I do not understand what the gcc_assert is doing
>>> in:
>>> +
>>> +      /* look for the corresponding scalarizer dimension: dim.  */
>>> +      for (dim = 0; dim < ndim; dim++)
>>> +        if (info->dim[dim] == n)
>>> +          break;
>>> +      gcc_assert (dim < ndim);
>>>
>>> Have I missed something?
>>
>> This seems like checking the loop is exited early in any case.  (But
>> I've no idea about the rest of the patch.)
>>
>> Daniel
>>
>> --
>> http://www.pro-vegan.info/
>> --
>> Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
>> To go: Hea-Mon-Pri
>>
>
>
> --
> The knack of flying is learning how to throw yourself at the ground and miss.
>       --Hitchhikers Guide to the Galaxy
>
Tobias Burnus - Sept. 21, 2010, 2:54 p.m.
On 09/21/2010 04:46 PM, Paul Richard Thomas wrote:
> The loop takes dim from 0 to ndim-1.  Therefore the condition of the
> gcc_assert is always going to be satisfied.

I disagree. The assert triggers

a) If ndim < 1 (and the loop is never entered)

b) If one does not have an early exit - as Daniel pointed out:

#include <stdio.h>
main () {
  int i;
  for (i = 0; i < 5; i++)
    ;
  printf("%d\n", i);
  return 0;
}

Running the program prints "5".

Tobias

> On 9/21/10, Daniel Kraft<d@domob.eu>  wrote:
>> Paul Richard Thomas wrote:
>>> +	  for (dim = 0; dim<  ndim; dim++)
>>> +	    if (info->dim[dim] == n)
>>> +	      break;
>>> +	  gcc_assert (dim<  ndim);
>> This seems like checking the loop is exited early in any case.  (But
>> I've no idea about the rest of the patch.)
Paul Richard Thomas - Sept. 21, 2010, 3:03 p.m.
Dear All,


> b) If one does not have an early exit - as Daniel pointed out:

duuuuh!

Paul
Mikael Morin - Sept. 21, 2010, 3:41 p.m.
On Tuesday 21 September 2010 17:03:54 Paul Richard Thomas wrote:
> Dear All,
> 
> > b) If one does not have an early exit - as Daniel pointed out:
> duuuuh!
> 
> Paul

Hello all,

yes, the assert checks the loop was exited early, and thus the dim looked for 
has been found. 
Would there be a way to present this more clearly ? How ?

Mikael
Mikael Morin - Sept. 21, 2010, 4:06 p.m.
On Tuesday 21 September 2010 17:41:15 Mikael Morin wrote:
> On Tuesday 21 September 2010 17:03:54 Paul Richard Thomas wrote:
> > Dear All,
> > 
> > > b) If one does not have an early exit - as Daniel pointed out:
> > duuuuh!
> > 
> > Paul
> 
> Hello all,
> 
> yes, the assert checks the loop was exited early, and thus the dim looked
> for has been found.
> Would there be a way to present this more clearly ? How ?
I will add a comment.
Steve Kargl - Sept. 21, 2010, 4:10 p.m.
On Tue, Sep 21, 2010 at 05:41:15PM +0200, Mikael Morin wrote:
> On Tuesday 21 September 2010 17:03:54 Paul Richard Thomas wrote:
> > Dear All,
> > 
> > > b) If one does not have an early exit - as Daniel pointed out:
> > duuuuh!
> > 
> > Paul
> 
> Hello all,
> 
> yes, the assert checks the loop was exited early, and thus the dim looked for 
> has been found. 
> Would there be a way to present this more clearly ? How ?
> 

Well, C has a feature called a comment.

   /* Assert that the loop exited early. */
   gcc_assert (...);

:-)
Paul Richard Thomas - Sept. 21, 2010, 6:25 p.m.
Steve, Mikael,

I am not sure that it is necessary to compensate for my suffering
temporary brain failure :-)  It is about as obvious as can be.....

Cheers

Paul

On Tue, Sep 21, 2010 at 6:10 PM, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> On Tue, Sep 21, 2010 at 05:41:15PM +0200, Mikael Morin wrote:
>> On Tuesday 21 September 2010 17:03:54 Paul Richard Thomas wrote:
>> > Dear All,
>> >
>> > > b) If one does not have an early exit - as Daniel pointed out:
>> > duuuuh!
>> >
>> > Paul
>>
>> Hello all,
>>
>> yes, the assert checks the loop was exited early, and thus the dim looked for
>> has been found.
>> Would there be a way to present this more clearly ? How ?
>>
>
> Well, C has a feature called a comment.
>
>   /* Assert that the loop exited early. */
>   gcc_assert (...);
>
> :-)
>
> --
> Steve
>

Patch

diff --git a/trans-array.c b/trans-array.c
index 7bce2ef..f15b53e 100644
--- a/trans-array.c
+++ b/trans-array.c
@@ -5439,7 +5439,6 @@  gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	}
 
       offset = gfc_index_zero_node;
-      dim = 0;
 
       /* The following can be somewhat confusing.  We have two
          descriptors, a new one and the original array.
@@ -5479,9 +5478,6 @@  gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	    }
 	  else
 	    {
-	      /* Check we haven't somehow got out of sync.  */
-	      gcc_assert (info->dim[dim] == n);
-
 	      /* Evaluate and remember the start of the section.  */
 	      start = info->start[n];
 	      stride = gfc_evaluate_now (stride, &loop.pre);
@@ -5505,6 +5501,12 @@  gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	  /* Vector subscripts need copying and are handled elsewhere.  */
 	  if (info->ref)
 	    gcc_assert (info->ref->u.ar.dimen_type[n] == DIMEN_RANGE);
+ 
+	  /* look for the corresponding scalarizer dimension: dim.  */
+	  for (dim = 0; dim < ndim; dim++)
+	    if (info->dim[dim] == n)
+	      break;
+	  gcc_assert (dim < ndim);
 
 	  /* Set the new lower bound.  */
 	  from = loop.from[dim];
@@ -5559,8 +5561,6 @@  gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
 	  /* Store the new stride.  */
 	  gfc_conv_descriptor_stride_set (&loop.pre, parm,
 					  gfc_rank_cst[dim], stride);
-
-	  dim++;
 	}
 
       if (se->data_not_needed)