Ping! (What do we do with this little bugger?) @Paul: Was your comment 19 in the PR meant as an OK for my patch (submitted here: http://gcc.gnu.org/ml/fortran/2012-12/msg00097.html), or was it just a general agreement with the previous comments?
Cheers, Janus 2012/12/16 Janus Weil <ja...@gcc.gnu.org>: > 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
