From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf36.google.com (mail-qv1-xf36.google.com [IPv6:2607:f8b0:4864:20::f36]) by sourceware.org (Postfix) with ESMTPS id 940C83850432 for ; Fri, 23 Jul 2021 16:26:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 940C83850432 Received: by mail-qv1-xf36.google.com with SMTP id hu11so1588458qvb.7 for ; Fri, 23 Jul 2021 09:26:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=11anfxBI5Ma/52PZ94gA4F5ziX33xlxa8IZ3NjoBYGc=; b=SqwAZxlChkp6UUYDFBj4gzAFYkY3qVBlu4hc5FgWw/jgD2pkx9ZFNAutvHsxSTP8xs FU1qpHg9M1wvDKgjFPUb3IDCTiiZMmuZQrsAgFgBqhjllbR0he/mgU+pjhe+NJY+cO70 HlOegRvJd1wJdUhryovZjsX5WN8P4+brbTMQmTl+bWk33MFHR8RbnLiOSM8mbCi6oxbN r2uNOsQUnBnk8NAMqOTN6JqPdk0Sk2QrSVeoDty/K4utOhuF+vpgjRtDNy4gFN+DoiW2 r5QMjyIBDxrVks7TB/S/3ky6p+m8rBWJ0tGywl8xw0NvcI2ofVmF0ICAErlEp1cWe7MT d85g== X-Gm-Message-State: AOAM533ErFnPfJvCdW15y0NQm0HNrkXL54QWtGCUpNRKXiJxEs5Yv+vj j8MPlGnjaHY7xRdh4+UVSOE= X-Google-Smtp-Source: ABdhPJwW1B7sHj0V5UUGRL0xAcdgRUM6gVKCEv8X4RzBG8wi9cNvHZF0V2vUj30NbpG4qwr5Qf0XBQ== X-Received: by 2002:a05:6214:1bcf:: with SMTP id m15mr5356413qvc.55.1627057582147; Fri, 23 Jul 2021 09:26:22 -0700 (PDT) Received: from [192.168.0.41] (75-166-102-22.hlrn.qwest.net. [75.166.102.22]) by smtp.gmail.com with ESMTPSA id 75sm14523650qkl.31.2021.07.23.09.26.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 23 Jul 2021 09:26:21 -0700 (PDT) Subject: Re: [PATCH] Make loops_list support an optional loop_p root To: "Kewen.Lin" , Richard Biener Cc: GCC Patches , Jakub Jelinek , Jonathan Wakely , Segher Boessenkool , Richard Sandiford , Trevor Saunders , Bill Schmidt References: <0a8b77ba-1d54-1eff-b54d-d2cb1e769e09@linux.ibm.com> <61ac669c-7293-f53a-20c7-158b5a813cee@linux.ibm.com> From: Martin Sebor Message-ID: <0fd941f1-a69a-ad2b-3dea-75dcc50a747b@gmail.com> Date: Fri, 23 Jul 2021 10:26:20 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <61ac669c-7293-f53a-20c7-158b5a813cee@linux.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Jul 2021 16:26:24 -0000 On 7/23/21 2:41 AM, Kewen.Lin wrote: > on 2021/7/22 下午8:56, Richard Biener wrote: >> On Tue, Jul 20, 2021 at 4:37 >> PM Kewen.Lin wrote: >>> >>> Hi, >>> >>> This v2 has addressed some review comments/suggestions: >>> >>> - Use "!=" instead of "<" in function operator!= (const Iter &rhs) >>> - Add new CTOR loops_list (struct loops *loops, unsigned flags) >>> to support loop hierarchy tree rather than just a function, >>> and adjust to use loops* accordingly. >> >> I actually meant struct loop *, not struct loops * ;) At the point >> we pondered to make loop invariant motion work on single >> loop nests we gave up not only but also because it iterates >> over the loop nest but all the iterators only ever can process >> all loops, not say, all loops inside a specific 'loop' (and >> including that 'loop' if LI_INCLUDE_ROOT). So the >> CTOR would take the 'root' of the loop tree as argument. >> >> I see that doesn't trivially fit how loops_list works, at least >> not for LI_ONLY_INNERMOST. But I guess FROM_INNERMOST >> could be adjusted to do ONLY_INNERMOST as well? >> > > > Thanks for the clarification! I just realized that the previous > version with struct loops* is problematic, all traversal is > still bounded with outer_loop == NULL. I think what you expect > is to respect the given loop_p root boundary. Since we just > record the loops' nums, I think we still need the function* fn? > So I add one optional argument loop_p root and update the > visiting codes accordingly. Before this change, the previous > visiting uses the outer_loop == NULL as the termination condition, > it perfectly includes the root itself, but with this given root, > we have to use it as the termination condition to avoid to iterate > onto its possible existing next. > > For LI_ONLY_INNERMOST, I was thinking whether we can use the > code like: > > struct loops *fn_loops = loops_for_fn (fn)->larray; > for (i = 0; vec_safe_iterate (fn_loops, i, &aloop); i++) > if (aloop != NULL > && aloop->inner == NULL > && flow_loop_nested_p (tree_root, aloop)) > this->to_visit.quick_push (aloop->num); > > it has the stable bound, but if the given root only has several > child loops, it can be much worse if there are many loops in fn. > It seems impossible to predict the given root loop hierarchy size, > maybe we can still use the original linear searching for the case > loops_for_fn (fn) == root? But since this visiting seems not so > performance critical, I chose to share the code originally used > for FROM_INNERMOST, hope it can have better readability and > maintainability. I might be mixing up the two patches (they both seem to touch the same functions), but in this one the loops_list ctor looks like a sizeable function with at least one loop. Since the ctor is used in the initialization of each of the many range-for loops, that could result in inlining of a lot of these calls and so quite a bit code bloat. Unless this is necessary for efficiency (not my area) I would recommend to consider defining the loops_list ctor out-of-line in some .c or .cc file. (Also, if you agree with the rationale, I'd replace loop_p with loop * in the new code.) Thanks Martin > > Bootstrapped and regtested on powerpc64le-linux-gnu P9, > x86_64-redhat-linux and aarch64-linux-gnu, also > bootstrapped on ppc64le P9 with bootstrap-O3 config. > > Does the attached patch meet what you expect? > > BR, > Kewen > ----- > gcc/ChangeLog: > > * cfgloop.h (loops_list::loops_list): Add one optional argument root > and adjust accordingly. >