This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch, Fortran] PR 55072: [4.6/4.7/4.8 Regression] Missing internal_pack leads to wrong code with derived type


Hi Tobias,

>> here is a patch for a pretty bad wrong-code regression, which affects
>> all maintained releases of gfortran. For discussion see bugzilla.
>>
>> 2012-12-15  Janus Weil<janus@gcc.gnu.org>
>>      PR fortran/55072
>>      * trans-array.c (gfc_conv_array_parameter): No packing was done for
>>      full arrays of derived type.
>>
>> @@ -6995,20 +6995,14 @@ gfc_conv_array_parameter (gfc_se * se, gfc_expr *
>>       this_array_result = false;
>>       /* Passing address of the array if it is not pointer or
>> assumed-shape.  */
>> -  if (full_array_var && g77 && !this_array_result)
>> +  if (full_array_var && g77 && !this_array_result
>> +      && sym->ts.type != BT_DERIVED && sym->ts.type != BT_CLASS)
>
> Without experimenting more carefully, I have the gut feeling that there are
> still cases where we might end up with invalid code and there is missed
> optimization.
>
> Regarding the latter: If the variable is simply contiguous, there is no need
> to pack it. Hence, for
>
> type(t), allocatable :: a(:)
> ...
> call bar(a)
>
> there is no need to pack the array.

Hm, ok. Do we do any packing in this case? Anyway, this sort of
missed-optimization issue can be treated in a follow-up patch, I
guess. (Mikael also noted a similar missed-optimization case in
comment #13 of the PR.)

What I'm aiming for here is primarily a patch for the wrong-code
regression which is suitable for all three maintained branches.


> The problem with the original test case
> is that one has a non-CONTIGUOUS pointer:
>
> p => tgt(::2,::2)
> call bar(p)
>
> But that has in principle nothing to do with BT_DERIVED.

Yes, the reason for the patch to handle BT_DERIVED in particular, is
that the original commit which introduced the regression (i.e.
r156749) messed up things for BT_DERIVED, which is what I'm reverting.


> In particular, I
> would like to see an additional test case for the first example case with
> "ptr" having the CONTIGUOUS attribute - and a check that then no packing
> call is invoked.

I just checked this: The patched trunk seems to handle this properly
and does not do any packing.

However, I think there might be another problem:

implicit none
type t
  integer :: i
end type t
type(t), target :: tgt(4,4)
type(t), pointer, contiguous :: p(:,:)
p => tgt(::2,::2)        ! accepts invalid?
end

The pointer assignment of a contiguous pointer to a non-contiguous
target should probably be rejected, right? Another follow-up problem
...


> For the second test case (comment 2, from GiBUU): Here, the problem is that
> "full_array_var" is wrongly true:
>
>   call s1(OutPart(1,:))
>
> I wonder whether some call to gfc_is_simply_contiguous could solve the
> problem for both issues.

No, here I disagree. The problem with this one was not related to the
call of "s1", but of "s2", where indeed a full array is passed!



> (For non-whole arrays one still have to ensure that one passes the correct
> element: "call(a)" should pass a->data and not "&a" and "call bar(a(:,2))"
> should neither pass "a->data" nor "&a" but "a->data + offset".)
>
> Regarding BT_CLASS: BT_CLASS -> BT_TYPE (with same declared type) should
> already be handled via gfc_conv_subref_array_arg, which takes care of the
> actual type. Thus, the patched function should only be reachable for
> BT_CLASS -> BT_CLASS. Here, packing is required for non-simply contiguous
> actual arguments; but after the packing, a class container has to be
> re-added. I think one should add a test case for this; testing declared type
> == actual type and declared type != actual type - and either one for both
> declared type being the same and for the dummy having the declared type of
> the ancestor of the declared type of the actual argument. And all cases for
> both simply contiguous arrays and (simply - or better actually)
> noncontiguous arrays.

I'm ignoring all this for now. All I want to fix at this point is the
wrong-code regression!


> Regarding the wrong code: I fear that some code involving non-BT_DERIVED
> could lead to wrong code, e.g. "a(:)%x". I don't have an example for that

If you find an example where stuff goes wrong (as a regression of my
patch), I'll take care of it.


> but I fear that code which lead to the original issue (e.g. "full_array_var"
> is true although it shouldn't) is not solved via the patch.

I actually don't think this is the case!


> Sorry for listing more my concerns that giving a proper review.

Thanks for your comments, anyway.

Cheers,
Janus


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]