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

Submitted by Mikael Morin on Sept. 20, 2010, 10:21 p.m.

Details

Message ID 20100920222130.27269.19670@gimli.local
State New
Headers show

Commit Message

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.

Comments

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 hide | download patch | download mbox

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)