Re: Request for help with the scalarizer

[Mikael Morin <mikael dot morin at sfr dot fr>]

On 22/08/2012 19:19, Tobias Burnus wrote:
> Dear all,
> 
> first, a question to Mikael (and others knowing the scalarizer): How to
> properly fix the following:
> 
> implicit none
> REAL qss(3)
> REAL, ALLOCATABLE :: qj(:,:)
> INTEGER             :: qcount
> qss(:)=qj(:,qcount)
> end
> 
> For that one calls gfc_cleanup_loop (&loop) - and in gfc_free_ss:
> 
>     case GFC_SS_SECTION:
>       for (n = 0; n < ss->dimen; n++)
>         {
>           if (ss_info->data.array.subscript[ss->dim[n]])
>             gfc_free_ss_chain (ss_info->data.array.subscript[ss->dim[n]]);
>         }
> 
> The problem is:
> 
> (gdb) p  ss->dimen
> $8 = 1
that's fine: rank 1 array

> (gdb) p ss->dim[0]
> $9 = 0
that's fine: the first dimension of the array subreference is the first
dimension of the full array.

> (gdb) p ss->info->data.array.subscript
> $10 = {0x0, 0x15f37f0, 0x0, 0x0, 0x0, 0x0, 0x0}
that's fine: there is a (scalar) subscript ss in dimension 1
corresponding to `qcount', and nothing in dimension 0 as there is no
vector subscript in that dimension.

> 
> The question is now whether ss->dim[0] should be 1 instead of 0, then
> the bug is in gfc_walk_array_ref's AR_SECTION: -> DIMEN_ELEMENT
> handling. 
No, it's fine as is.

> Or whether the gfc_free_ss handling is wrong. A brute-force
> method would be to walk all MAX_DIMENSION elements of
> ss->info->data.array.subscript.
Yes, that's the way it should be. For example gfc_add_loop_ss_code has
already one "brute force" loop about subscripts:

	case GFC_SS_SECTION:
	  /* Add the expressions for scalar and vector subscripts.  */
	  for (n = 0; n < GFC_MAX_DIMENSIONS; n++)
	    if (info->subscript[n])
	      gfc_add_loop_ss_code (loop, info->subscript[n], true, where);


> 
> 
> Secondly, I tried to to fix all gfc_ss mem leaks (including PR54350,
> which I accidentally introduced).
> 
> The attached patch works nicely for the test suite (except for
> realloc_on_assign_*.f90 aka PR54350), it also fixes the leaks in some
> real-world test  files. And it compiles nearly all polyhedron examples.
> 
> However: It fails to compile rnflow of Polyhedron 2005. Namely, one
> enters an endless loop in gfc_conv_ss_startstride with the following
> backtrace. Obviously, one has freed too much memory to early. Namely:
> 
>   ss = gfc_walk_expr (expr1);
>   gfc_conv_array_parameter (&se, expr1, ss, false, NULL, NULL, NULL);
>           realloc_lhs_loop_for_fcn_call (&se, &expr1->where, &ss, &loop);
> 
> With the current patch, gfc_conv_array_parameter always frees "ss";
> before, it only freed ss by calling gfc_conv_expr_descriptor.
> 
> 
> How to solve that? A partial ss freeing is rather bad as one cannot
> detect whether "ss" has been freed or not. One solution would be that
> gfc_conv_expr_descriptor no longer frees the memory - i.e. the caller
> has to do the duty. That's probably the most invasive patch, but at
> least it makes the code clearer.
In general, I prefer having allocation and deallocation at the same
scope for clarity. In gfc_conv_expr_descriptor, it makes some sense to
throw away ss once it has been used for a loop, so it would be better to
have the allocation happen there too.
I have reviewed gfc_conv_expr_descriptor calls, and unless I missed
some, they all follow the same pattern:
ss = gfc_walk_expr (expr);
...
gfc_init_se (&se, ...);
...
gfc_conv_expr_descriptor (&se, expr, ss);

This shows that ss is redundant with expr, so I think the walking should
be internal to gfc_conv_expr_descriptor, the ss argument should be
removed, and then gfc_conv_array_parameter doesn't need ss any more. I
believe it would solve your problem, and make allocation and cleanup
happen in the same function, which is nice. I don't think it would avoid
invasive changes though.

Mikael