From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12320 invoked by alias); 14 Nov 2019 09:39:49 -0000 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 Received: (qmail 12304 invoked by uid 89); 14 Nov 2019 09:39:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-5.9 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 spammy=gathering, fes X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 14 Nov 2019 09:39:47 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 7A769AF98; Thu, 14 Nov 2019 09:39:45 +0000 (UTC) Subject: Re: [PATCH] Add if-chain to switch conversion pass. To: Richard Biener , Jakub Jelinek Cc: GCC Patches , kazu@gcc.gnu.org, Jan Hubicka References: <2c3db526-cac6-4eeb-4afb-12024f8d5af2@suse.cz> <20191104144851.GJ4650@tucnak> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <6169f91a-4884-55f5-c76f-ea1dae11d996@suse.cz> Date: Thu, 14 Nov 2019 09:41:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-11/txt/msg01133.txt.bz2 On 11/5/19 1:38 PM, Richard Biener wrote: > On Mon, Nov 4, 2019 at 3:49 PM Jakub Jelinek wrote: >> >> On Mon, Nov 04, 2019 at 03:23:20PM +0100, Martin Liška wrote: >>> The patch adds a new pass that identifies a series of if-elseif >>> statements and transform then into a GIMPLE switch (if possible). >>> The pass runs right after tree-ssa pass and I decided to implement >>> matching of various forms that are introduced by folder (fold_range_test): >> >> Not a review, just a few questions: Hello. > > Likewise - please do not name switches -ftree-*, 'tree' doens't add anything > but confusion to users. Thus use -fif-to-switch or -fconvert-if-to-switch Agree with you, I selected the later option. > > +The transformation can help to produce a faster code for > +the switch statement. > > produce faster code. Fixed. > > Doesn't it also produce smaller code eventually? In some situation yes, but generally it leads to more jump tables (which are bigger when expanded). > > Please to not put code transform passes into build_ssa_passes (why did > you choose this place)? Well, that was my initial pass selection as I wanted to have a GIMPLE code close to what FEs produce. > The pass should go into pass_all_early_optimizations > instead, and I'm quite sure you want to run _after_ CSE. I'd even say > that the pass should run as part of switch-conversion, so we build > a representation of a switch internally and then code-generate the optimal > form directly. For now just put the pass before switch-conversion. But yes, the suggested place is much better place and we can benefit from VRP (that will kill dead conditions in a if-else chain) > > There are functions without comments in the patch and you copied > from DSE which shows in confusing comments left over from the original. > > + mark_virtual_operands_for_renaming (cfun); > > if you did nothing renaming all vops is expensive. This one is needed for situations like: fn1 () { a.0_1; : # VUSE <.MEM_5(D)> a.0_1 = a; if (a.0_1 == 0) goto ; [INV] else goto ; [INV] : if (a.0_1 == 1) goto ; [INV] else goto ; [INV] : if (a.0_1 == 2) goto ; [INV] else goto ; [INV] : # .MEM_6 = VDEF <.MEM_5(D)> fn2 (); : # .MEM_4 = PHI <.MEM_5(D)(2), .MEM_5(D)(3), .MEM_5(D)(4), .MEM_6(5)> # VUSE <.MEM_4> return; } where without the call, I end up with: : # VUSE <.MEM_5(D)> a.0_1 = a; switch (a.0_1) [INV], case 0: [INV], case 1: [INV], case 2: [INV]> : : # .MEM_6 = VDEF <.MEM_5(D)> fn2 (); : # .MEM_4 = PHI <.MEM_6(3), (2)> > > I'm missing an overall comment - you are using a dominator walk > but do nothing in the after hook which means you are not really > gathering any data? You're also setting visited bits on BBs which > means you are visiting alternate BBs during the DOM walk. You are right, I'm a bit cheating with the DOM walk as I also mark visited BBs. What I want is for each if-else condition, I want to visit the first IF in such chain. That's why I decided to iterate in DOMINATOR order. Can I do it simpler? Thanks, Martin > >> 1) what does it do if __builtin_expect* has been used, does it preserve >> the probabilities and if in the end decides to expand as ifs, are those >> probabilities retained through it? >> 2) for the reassoc-*.c testcases, do you get identical or better code >> with the patch? >> 3) shouldn't it be gimple-if-to-switch.c instead? >> 4) what code size effect does the patch have say on cc1plus (if you don't >> count the code changes of the patch itself, i.e. revert the patch in the >> stage3 and rebuild just the stage3)? >> >>> +struct case_range >>> +{ >>> + /* Default constructor. */ >>> + case_range (): >>> + m_min (NULL_TREE), m_max (NULL_TREE) >> >> I admit I'm never sure about coding conventions for C++, >> but shouldn't there be a space before :, or even better : >> be on the next line before m_min ? >> >> Jakub >>