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<ja...@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
