From patchwork Tue May 19 21:39:10 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Prathamesh Kulkarni X-Patchwork-Id: 48776 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-wg0-f70.google.com (mail-wg0-f70.google.com [74.125.82.70]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id BC52121411 for ; Tue, 19 May 2015 21:39:39 +0000 (UTC) Received: by wgbgf7 with SMTP id gf7sf8631963wgb.2 for ; Tue, 19 May 2015 14:39:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mailing-list:precedence:list-id :list-unsubscribe:list-archive:list-post:list-help:sender :delivered-to:mime-version:in-reply-to:references:date:message-id :subject:from:to:cc:content-type:x-original-sender :x-original-authentication-results; bh=+f0VEx/pF9McWf6+1Vvk7ptOME4DvCae2ljbKwamwXs=; b=DuV4q4eG5ZLYWOEKViqjyVqKGj4pnG98/iwmV3Tc/UDBvmB6ZHRUZxxtmATVtURftG A+DoyZmfm2MWxdLAryUt+HaY/qgOWB7C4OOT3pKJI6KeuvkoUOGQ20NBap1Zn8VKUsTP Hghe7m6YmoGTuFd798+LWJhvUXp44gbJwv8wxMTb0qo08eFu+HYW0gp8dVfBqvTYsWz8 4+hw3yqYedHHfolaOa/tuJZz2A9wg78dlOtIVppPXmbcnravUEmj8T+I+6jpji4kUmBP Pbd1m1nz6tISnILK1sOULhYqk1ZdmhJXM/5WwW/y7ir1yZBUqWdStGhdFG3UuxKu4NmU wu/w== X-Gm-Message-State: ALoCoQkAzKyFh94uV7b+EJ4MgWwvAJhCD6nnJfuyLyBuu9vIpBNonmvqL66rOb6LZMGmkmrc25U5 X-Received: by 10.194.202.229 with SMTP id kl5mr10134152wjc.3.1432071578110; Tue, 19 May 2015 14:39:38 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.44.135 with SMTP id e7ls129679lam.79.gmail; Tue, 19 May 2015 14:39:37 -0700 (PDT) X-Received: by 10.152.22.72 with SMTP id b8mr7196939laf.1.1432071577940; Tue, 19 May 2015 14:39:37 -0700 (PDT) Received: from mail-la0-x230.google.com (mail-la0-x230.google.com. [2a00:1450:4010:c03::230]) by mx.google.com with ESMTPS id le4si9874913lac.158.2015.05.19.14.39.37 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 19 May 2015 14:39:37 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 2a00:1450:4010:c03::230 as permitted sender) client-ip=2a00:1450:4010:c03::230; Received: by labbd9 with SMTP id bd9so45808865lab.2 for ; Tue, 19 May 2015 14:39:37 -0700 (PDT) X-Received: by 10.112.222.133 with SMTP id qm5mr23562041lbc.86.1432071577704; Tue, 19 May 2015 14:39:37 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.108.230 with SMTP id hn6csp1086334lbb; Tue, 19 May 2015 14:39:34 -0700 (PDT) X-Received: by 10.70.32.164 with SMTP id k4mr58359413pdi.138.1432071572952; Tue, 19 May 2015 14:39:32 -0700 (PDT) Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id f4si23232771pas.118.2015.05.19.14.39.31 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 19 May 2015 14:39:32 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-return-398322-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Received: (qmail 115958 invoked by alias); 19 May 2015 21:39:16 -0000 Mailing-List: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 115944 invoked by uid 89); 19 May 2015 21:39:16 -0000 X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL, BAYES_40, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-la0-f43.google.com Received: from mail-la0-f43.google.com (HELO mail-la0-f43.google.com) (209.85.215.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 19 May 2015 21:39:14 +0000 Received: by lagr1 with SMTP id r1so45046944lag.0 for ; Tue, 19 May 2015 14:39:10 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.112.48.68 with SMTP id j4mr158090lbn.60.1432071550356; Tue, 19 May 2015 14:39:10 -0700 (PDT) Received: by 10.25.205.146 with HTTP; Tue, 19 May 2015 14:39:10 -0700 (PDT) In-Reply-To: References: Date: Wed, 20 May 2015 03:09:10 +0530 Message-ID: Subject: Re: [match-and-simplify] fix incorrect code-gen in 'for' pattern From: Prathamesh Kulkarni To: Richard Biener Cc: gcc Patches X-IsSubscribed: yes X-Original-Sender: prathamesh.kulkarni@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 2a00:1450:4010:c03::230 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org; dkim=pass header.i=@gcc.gnu.org X-Google-Group-Id: 836684582541 On 19 May 2015 at 14:34, Richard Biener wrote: > On Tue, 19 May 2015, Prathamesh Kulkarni wrote: > >> On 18 May 2015 at 20:17, Prathamesh Kulkarni >> wrote: >> > On 18 May 2015 at 14:12, Richard Biener wrote: >> >> On Sat, 16 May 2015, Prathamesh Kulkarni wrote: >> >> >> >>> Hi, >> >>> genmatch generates incorrect code for following (artificial) pattern: >> >>> >> >>> (for op (plus) >> >>> op2 (op) >> >>> (simplify >> >>> (op @x @y) >> >>> (op2 @x @y) >> >>> >> >>> generated gimple code: http://pastebin.com/h1uau9qB >> >>> 'op' is not replaced in the generated code on line 33: >> >>> *res_code = op; >> >>> >> >>> I think it would be a better idea to make op2 iterate over same set >> >>> of operators (op2->substitutes = op->substitutes). >> >>> I have attached patch for the same. >> >>> Bootstrap + testing in progress on x86_64-unknown-linux-gnu. >> >>> OK for trunk after bootstrap+testing completes ? >> >> >> >> Hmm, but then the example could as well just use 'op'. I think we >> >> should instead reject this. >> >> >> >> Consider >> >> >> >> (for op (plus minus) >> >> (for op2 (op) >> >> (simplify ... >> >> >> >> where it is not clear what would be desired. Simple replacement >> >> of 'op's value would again just mean you could have used 'op' in >> >> the first place. Doing what you propose would get you >> >> >> >> (for op (plus minus) >> >> (for op2 (plus minus) >> >> (simplify ... >> >> >> >> thus a different iteration. >> >> >> >>> I wonder if we really need is_oper_list flag in user_id ? >> >>> We can determine if user_id is an operator list >> >>> if user_id::substitutes is not empty ? >> >> >> >> After your change yes. >> >> >> >>> That will lose the ability to distinguish between user-defined operator >> >>> list and list-iterator in for like op/op2, but I suppose we (so far) don't >> >>> need to distinguish between them ? >> >> >> >> Well, your change would simply make each list-iterator a (temporary) >> >> user-defined operator list as well as the current iterator element >> >> (dependent on context - see the nested for example). I think that >> >> adds to confusion. >> AFAIU, the way it's implemented in lower_for, the iterator is handled >> the same as a user-defined operator >> list. I was wondering if we should get rid of 'for' altogether and >> have it replaced >> by operator-list ? >> >> IMHO having two different things - iterator and operator-list is >> unnecessary and we could >> brand iterator as a "local" operator-list. We could extend syntax of 'simplify' >> to accommodate "local" operator-lists. >> >> So we can say, using an operator-list within 'match' replaces it by >> corresponding operators in that list. >> Operator-lists can be "global" (visible to all patterns), or local to >> a particular pattern. >> >> eg: >> a) single for >> (for op (...) >> (simplify >> (op ...))) >> >> can be written as: >> (simplify >> op (...) // define "local" operator-list op. >> (op ...)) // proceed here the same way as for lowering "global" operator list. > > it's not shorter and it's harder to parse. And you can't share the > operator list with multiple simplifies like > > (for op (...) > (simplify > ...) > (simplify > ...)) > > which is already done I think. I missed that -;) Well we can have a "workaround syntax" for that if desired. > >> b) multiple iterators: >> (for op1 (...) >> op2 (...) >> (simplify >> (op1 (op2 ...)))) >> >> can be written as: >> (simplify >> op1 (...) >> op2 (...) >> (op1 (op2 ...))) >> >> c) nested for >> (for op1 (...) >> (for op2 (...) >> (simplify >> (op1 (op2 ...)))) >> >> can be written as: >> >> (simplify >> op1 (...) >> (simplify >> op2 (...) >> (op1 (op2 ...)))) >> >> My rationale behind removing 'for' is we don't need to distinguish >> between an "operator-list" and "iterator", >> and only have an operator-list -;) >> Also we can reuse parser::parse_operator_list (in parser::parse_for >> parsing oper-list is duplicated) >> and get rid of 'parser::parse_for'. >> We don't need to change lowering, since operator-lists are handled >> the same way as 'for' (we can keep lowering of simplify::for_vec as it is). >> >> Does it sound reasonable ? > > I dont' think the proposed syntax is simpler or more powerful. Hmm I tend to agree. My motivation to remove 'for' was that it is not more powerful than operator-list and we can re-write 'for' with equivalent operator-list with some syntax changes (like putting operator-list in simplify etc.) So there's only one of doing the same thing. > > Richard. > >> Thanks, >> Prathamesh >> >> >> >> So - can you instead reject this use? I have attached patch for rejecting this use of iterator. Ok for trunk after bootstrap+testing ? Thanks, Prathamesh >> > Well my intention was to have support for walking operator list in reverse. >> > For eg: >> > (for bitop (bit_and bit_ior) >> > rbitop (bit_ior bit_and) >> > ...) >> > Could be replaced by sth like: >> > (for bitop (bit_and bit_ior) >> > rbitop (~bitop)) >> > ...) >> > >> > where "~bitop" would indicate walking (bit_and bit_ior) in reverse. >> > Would that be a good idea ? For symmetry, I thought >> > (for op (list) >> > op2 (op)) >> > should be supported too. >> > >> > >> > Thanks, >> > Prathamesh >> > >> > >> >> >> >> Thanks, >> >> Richard. >> >> > > -- > Richard Biener > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg) 2015-05-20 Prathamesh Kulkarni * genmatch.c (parser::parse_for): Reject iterator if used as operator-list. Index: genmatch.c =================================================================== --- genmatch.c (revision 223352) +++ genmatch.c (working copy) @@ -3329,8 +3329,13 @@ "others with arity %d", oper, idb->nargs, arity); user_id *p = dyn_cast (idb); - if (p && p->is_oper_list) - op->substitutes.safe_splice (p->substitutes); + if (p) + { + if (p->is_oper_list) + op->substitutes.safe_splice (p->substitutes); + else + fatal_at (token, "iterator cannot be used as operator-list"); + } else op->substitutes.safe_push (idb); }