Re: [Patch, Fortran] PR 55072: [4.6/4.7/4.8 Regression] Missing internal_pack leads to wrong code with derived type
OK by me for trunk, followed by 4.6 and 4.7. Thanks, Paul. Committed to trunk as r195125. Will do the branches soon. Cheers, Janus On 11 January 2013 21:51, Mikael Morin mikael.mo...@sfr.fr wrote: Le 11/01/2013 21:31, Janus Weil a écrit : Thanks for your review (which I read as an OK for all branches, right?). Well, from my point of view it is OK, but I was actually hoping Tobias would ack it himself. -- The knack of flying is learning how to throw yourself at the ground and miss. --Hitchhikers Guide to the Galaxy
Re: [Patch, Fortran] PR 55072: [4.6/4.7/4.8 Regression] Missing internal_pack leads to wrong code with derived type
Le 10/01/2013 20:39, Janus Weil a écrit : 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? FWIW, I regard the patch as a (conservative) improvement, thus certainly acceptable. For: @@ -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) I would have used instead: expr-expr_type == EXPR_VARIABLE ref == NULL) to make the optimization available to variables of derived type. As we are now in stage4, your version should probably be preferred though. Regarding: 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 I don't think this can happen as the test for non-derived type is on the root symbol (`a' in your example). For other cases, to be honest, I can't make any sense of all the booleans interacting with each other in that code area (this_array_result, g77, contiguous vs. gfc_is_simply_contiguous, ...). Regarding the missed optimization, bah, maybe we can defer to 4.9+? Mikael
Re: [Patch, Fortran] PR 55072: [4.6/4.7/4.8 Regression] Missing internal_pack leads to wrong code with derived type
Hi Mikael, 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? FWIW, I regard the patch as a (conservative) improvement, thus certainly acceptable. To be conservative was exactly my intention, since a) trunk is in stage 4 and b) I wanted something that is safe for backporting to 4.6 and 4.7 (where we certainly can not afford to introduce any new wrong-code regressions). @@ -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) I would have used instead: expr-expr_type == EXPR_VARIABLE ref == NULL) to make the optimization available to variables of derived type. As we are now in stage4, your version should probably be preferred though. Ok, I will leave it as it is then. Regarding: 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 I don't think this can happen as the test for non-derived type is on the root symbol (`a' in your example). For other cases, to be honest, I can't make any sense of all the booleans interacting with each other in that code area (this_array_result, g77, contiguous vs. gfc_is_simply_contiguous, ...). Regarding the missed optimization, bah, maybe we can defer to 4.9+? Yes, certainly the upcoming release should better produce code that is fully correct, rather than fast but wrong ;) Thanks for your review (which I read as an OK for all branches, right?). Will commit soon. Cheers, Janus
Re: [Patch, Fortran] PR 55072: [4.6/4.7/4.8 Regression] Missing internal_pack leads to wrong code with derived type
Le 11/01/2013 21:31, Janus Weil a écrit : Thanks for your review (which I read as an OK for all branches, right?). Well, from my point of view it is OK, but I was actually hoping Tobias would ack it himself.
Re: [Patch, Fortran] PR 55072: [4.6/4.7/4.8 Regression] Missing internal_pack leads to wrong code with derived type
To be clear - I was awaiting a formal submission but indicating that I would OK it when it was made. I completely missed the posting of 16th December. OK by me for trunk, followed by 4.6 and 4.7. Cheers Paul On 11 January 2013 21:51, Mikael Morin mikael.mo...@sfr.fr wrote: Le 11/01/2013 21:31, Janus Weil a écrit : Thanks for your review (which I read as an OK for all branches, right?). Well, from my point of view it is OK, but I was actually hoping Tobias would ack it himself. -- The knack of flying is learning how to throw yourself at the ground and miss. --Hitchhikers Guide to the Galaxy
Re: [Patch, Fortran] PR 55072: [4.6/4.7/4.8 Regression] Missing internal_pack leads to wrong code with derived type
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 Weilja...@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
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 Weilja...@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
[Patch, Fortran] PR 55072: [4.6/4.7/4.8 Regression] Missing internal_pack leads to wrong code with derived type
Hi all, here is a patch for a pretty bad wrong-code regression, which affects all maintained releases of gfortran. For discussion see bugzilla. Regtested on x86_64-unknown-linux-gnu. Ok for 4.6, 4.7 and trunk? Cheers, Janus 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. 2012-12-15 Janus Weil ja...@gcc.gnu.org PR fortran/55072 * gfortran.dg/assumed_type_2.f90: Fix test case. * gfortran.dg/internal_pack_13.f90: New test. * gfortran.dg/internal_pack_14.f90: New test. pr55072_v4.diff Description: Binary data internal_pack_13.f90 Description: Binary data internal_pack_14.f90 Description: Binary data
