From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 06B0C3950C61 for ; Fri, 30 Jul 2021 09:34:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 06B0C3950C61 Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-124-_2_hmc-nP6WBG9fWGn11zg-1; Fri, 30 Jul 2021 05:34:55 -0400 X-MC-Unique: _2_hmc-nP6WBG9fWGn11zg-1 Received: by mail-wm1-f71.google.com with SMTP id o67-20020a1ca5460000b0290223be6fd23dso1429663wme.1 for ; Fri, 30 Jul 2021 02:34:55 -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:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=4cEB5nMuSJ0hNlT+iROW2deJQZCYst3s2rAq04q+K1M=; b=LH4I/RAM3qJMEZR6Ees2pHL+YBk0odYb82bjCBQ1n2HknpBoYpD0IGKWiCmustb/zg 1IqZ5CbjxoMV6CtgTgMw6yFrhfIJNrTSn10W+3mDz23a9JGi6eoIA/1xCub6nxy6o1Nz AxakSBbXlvBnGMN56mgZDl20R6gBbg5boO+oDjepzXkI0fADuQsDyjWLkup4M55hIvC6 KZnm5/sSTZ2CmUHJt4FOV43O7A6TEmqjMQnoWAJy+wgZZWmNpcabr+GM0WXAUsIQ8sbj oTz3cRnKRdpdAUiuMK8A24I1O2qkxytklK0IQIAG32xSTLkVPjcte72Ne5db+xomkFcy wGLg== X-Gm-Message-State: AOAM530x1fjsZp22DbPvxbXbhUbKGXHP9IGPwlWaEdmz+vpwy72CQjQ7 bFD9Py1+YYGr2r6VJ3Wklrs1bw8e2wcVMC6TuV69pWUiM+sK5M7qCS9e7vuoFpEaAvFgpCw6ttC vfy9yRLw/fWtYXlRs9Q== X-Received: by 2002:adf:9cc7:: with SMTP id h7mr2059962wre.406.1627637694653; Fri, 30 Jul 2021 02:34:54 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw50NXmbmLhcNhWD47bszxXZ4CjyBd7X4H0iOYO3wi6OJTaNn5qHjuoncIgqRw4PXQro6kc5w== X-Received: by 2002:adf:9cc7:: with SMTP id h7mr2059940wre.406.1627637694367; Fri, 30 Jul 2021 02:34:54 -0700 (PDT) Received: from abulafia.quesejoda.com ([95.169.226.92]) by smtp.gmail.com with ESMTPSA id a9sm1176627wrv.37.2021.07.30.02.34.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 30 Jul 2021 02:34:53 -0700 (PDT) Subject: Re: [PATCH] Replace evrp use in loop versioning with ranger. To: GCC patches , richard.sandiford@arm.com References: <20210724141937.2325339-1-aldyh@redhat.com> From: Aldy Hernandez Message-ID: Date: Fri, 30 Jul 2021 11:34:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=no 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, 30 Jul 2021 09:35:00 -0000 On 7/30/21 10:39 AM, Richard Sandiford wrote: > Aldy Hernandez writes: >> On Mon, Jul 26, 2021 at 7:28 PM Richard Sandiford >> wrote: >>> >>> Aldy Hernandez writes: >>>> On Mon, Jul 26, 2021 at 4:18 PM Richard Sandiford >>>> wrote: >>>>> >>>>> Aldy Hernandez writes: >>>>>> This patch replaces the evrp_range_analyzer in the loop versioning code >>>>>> with an on-demand ranger. >>>>>> >>>>>> Everything was pretty straightforward, except that range_of_expr requires >>>>>> a gimple statement as context to provide context aware ranges. I didn't see >>>>>> a convient place where the statement was saved, so I made a vector indexed >>>>>> by SSA names. As an alternative, I tried to use the loop's first statement, >>>>>> but that proved to be insufficient. >>>>> >>>>> The mapping is one-to-many though: there can be multiple statements >>>>> for each SSA name. Maybe that doesn't matter in this context and >>>>> any of the statements can act as a representative. >>>>> >>>>> I'm surprised that the loop's first statement didn't work though, >>>>> since the SSA name is supposedly known to be loop-invariant. What went >>>>> wrong when you tried that? >>>> >>>> I was looking at the first statement of loop_info->block_list and one >>>> of the dg.exp=loop-versioning* tests failed. Perhaps I should have >>>> used the loop itself, as in the attached patch. With this patch all >>>> of the loop-versioning tests pass. >>>> >>>>> >>>>>> I am not familiar with loop versioning, but if the DOM walk was only >>>>>> necessary for the calls to record_ranges_from_stmt, this too could be >>>>>> removed as the ranger will work without it. >>>>> >>>>> Yeah, that was the only reason. If the information is available at >>>>> version_for_unity (I guess it is) then we should just avoid recording >>>>> the versioning there if so. >>>>> >>>>> How expensive is the check? If the result is worth caching, perhaps >>>>> we should have two bitmaps: the existing one, and one that records >>>>> whether we've checked a particular SSA name. >>>>> >>>>> If the check is relatively cheap then that won't be worth it though. >>>> >>>> If you're asking about the range_of_expr check, that's all cached, so >>>> it should be pretty cheap. Besides, we're no longer calculating >>>> ranges for each statement in the IL, as we were doing in lv_dom_walker >>>> with evrp's record_ranges_from_stmt. Only statements of interest are >>>> queried. >>> >>> Sounds good. If the results are already cached then another level >>> of caching (via the second bitmap I mentioned above) would obviously >>> be a waste of time. >> >> My callgrind harness for performance testing wasn't able to pick up >> enough samples to measure the time spent in >> pass_loop_versioning::execute. I've seen this happen before with >> passes that run too fast. I'm afraid I don't have enough cycles to >> continue working on this. > > Yeah, any testing of this was above and beyond IMO. Hearing that the > range query does its own caching was enough for me. :-) > >>>> How about this patch, pending tests? >>> >>> OK, thanks, as a strict improvement over the status quo. But it'd be >>> even better without the dom walk :-) >> >> I've removed the DOM walk, and re-tested. >> >> OK to push? > > Sorry for asking for another iteration, but… It looks like this is a bit more involved than I originally envisioned. I've pushed the original (approved) patch that just removes the use of evrp, which was my main goal. I'll follow-up with the dom walk removal and your suggested changes next week when I have more cycles. Aldy