From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6657 invoked by alias); 14 Nov 2019 10:38:10 -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 6642 invoked by uid 89); 14 Nov 2019 10:38:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=H*i:sk:6169f91, H*f:sk:6169f91 X-HELO: mail-lj1-f195.google.com Received: from mail-lj1-f195.google.com (HELO mail-lj1-f195.google.com) (209.85.208.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 14 Nov 2019 10:38:08 +0000 Received: by mail-lj1-f195.google.com with SMTP id k15so6123571lja.3; Thu, 14 Nov 2019 02:38:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=KtInCCmZ22trr5Y/VqAXf0xf8nk4yEP5pVO8pELSAwE=; b=eRJHdPzEuvcvG5qJ7SU/eTJ+X3YWsUB0SFg3AFv7vHD4LZhzoNQeprwgbeXDDHXosE 4Tyddsyuo2X/J7Pu3coeS9xtVSLrXLYzVuampvH/Mhz46iENb5jcaZnQKEOx329OzyYO u0MKJk6/BSQaj0aCW1NE4FsOi1JKj7tFaSWIiBHm33XmzLF1r84LkBBLNoq36FEyo+S9 0ZPRm5xHXagRoHlF/cUdX2PkMBMS4tYTV4UAdgwuWYT0x6HFfcOIXIhLQaeGzg3+yfMw gP4aVBvIwGChUSIjwtFRFdaIaLLD/Ll0aDyrhmDJH8XAt090L/g0/8Pzq/nNZPw86mwr 1evw== MIME-Version: 1.0 References: <2c3db526-cac6-4eeb-4afb-12024f8d5af2@suse.cz> <20191104144851.GJ4650@tucnak> <6169f91a-4884-55f5-c76f-ea1dae11d996@suse.cz> In-Reply-To: <6169f91a-4884-55f5-c76f-ea1dae11d996@suse.cz> From: Richard Biener Date: Thu, 14 Nov 2019 10:48:00 -0000 Message-ID: Subject: Re: [PATCH] Add if-chain to switch conversion pass. To: =?UTF-8?Q?Martin_Li=C5=A1ka?= Cc: Jakub Jelinek , GCC Patches , kazu@gcc.gnu.org, Jan Hubicka Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2019-11/txt/msg01144.txt.bz2 On Thu, Nov 14, 2019 at 10:39 AM Martin Li=C5=A1ka wrote: > > 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=C5=A1ka 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_t= est): > >> > >> Not a review, just a few questions: > > Hello. > > > > > Likewise - please do not name switches -ftree-*, 'tree' doens't add any= thing > > but confusion to users. Thus use -fif-to-switch or -fconvert-if-to-swi= tch > > 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 opti= mal > > 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 =3D a; > if (a.0_1 =3D=3D 0) > goto ; [INV] > else > goto ; [INV] > > : > if (a.0_1 =3D=3D 1) > goto ; [INV] > else > goto ; [INV] > > : > if (a.0_1 =3D=3D 2) > goto ; [INV] > else > goto ; [INV] > > : > # .MEM_6 =3D VDEF <.MEM_5(D)> > fn2 (); > > : > # .MEM_4 =3D 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 =3D a; > switch (a.0_1) [INV], case 0: [INV], case 1: = [INV], case 2: [INV]> > > : > : > # .MEM_6 =3D VDEF <.MEM_5(D)> > fn2 (); > > : > # .MEM_4 =3D PHI <.MEM_6(3), (2)> I meant you are doing it unconditionally even if you didn't transform any if-then-else chain. > > > > > 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 visite= d BBs. > What I want is for each if-else condition, I want to visit the first IF i= n such > chain. That's why I decided to iterate in DOMINATOR order. > Can I do it simpler? Put that in a comment, do away with domwalk and instead start from ENTRY_BLOCK, using a worklist seeded by {first,next}_dom_son () and avoid putting visited blocks on that worklist. Btw, what about if-chains nested in another if-chain? Don't you want to transform "inner" chains first or does it really not matter (you're adjusti= ng the CFG, doing that inside the domwalk is fishy since that also uses pre-computed RPO order; the simple dom-son walking should work but you of course might miss some blocks depending on how you set up things). Richard. > 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 do= n'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 > >> >