From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14017 invoked by alias); 11 Jan 2013 20:32:04 -0000 Received: (qmail 13997 invoked by uid 22791); 11 Jan 2013 20:32:01 -0000 X-SWARE-Spam-Status: No, hits=-5.0 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-ie0-f182.google.com (HELO mail-ie0-f182.google.com) (209.85.223.182) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 11 Jan 2013 20:31:50 +0000 Received: by mail-ie0-f182.google.com with SMTP id s9so2819854iec.41 for ; Fri, 11 Jan 2013 12:31:47 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.50.194.167 with SMTP id hx7mr309150igc.99.1357936307625; Fri, 11 Jan 2013 12:31:47 -0800 (PST) Received: by 10.64.141.80 with HTTP; Fri, 11 Jan 2013 12:31:47 -0800 (PST) In-Reply-To: <50F04999.5080105@sfr.fr> References: <50CDB321.5060002@net-b.de> <50F04999.5080105@sfr.fr> Date: Fri, 11 Jan 2013 20:32:00 -0000 Message-ID: Subject: Re: [Patch, Fortran] PR 55072: [4.6/4.7/4.8 Regression] Missing internal_pack leads to wrong code with derived type From: Janus Weil To: Mikael Morin Cc: Tobias Burnus , gfortran , gcc-patches , Paul Thomas Content-Type: text/plain; charset=ISO-8859-1 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2013-01/txt/msg00625.txt.bz2 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