From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 76FDA3858436; Wed, 14 Jun 2023 07:59:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 76FDA3858436 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 477A622510; Wed, 14 Jun 2023 07:59:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1686729545; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=xRmLSTR6ZmOottrkCyhLNu5dybhHzb4kOXU63pNsW4E=; b=YOlvtEjx7/elmFUJN3OLh+WRvR40S0URcGnGRkAFQqJ2zeTl8O0fOxE5dJfZrYea9nhZu4 Qkcjcr1hqxNuHj8TClTb3PH38wff2q8Opsj2La6hio7Xd9LtJcJ2Uqw6EBNJkWkiT/KiAI dHMRIrupW4p9ssrJ3qyU+5wPCvp6gFM= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1686729545; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=xRmLSTR6ZmOottrkCyhLNu5dybhHzb4kOXU63pNsW4E=; b=kHb2JgpqIkHxGfDrk91HtNE+9n620coRBG/p+jzD4ARNsYKSDffS5+jFmy/BtqScMkr3T/ 1EhsSvLraJctqMAw== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 0B1322C141; Wed, 14 Jun 2023 07:59:04 +0000 (UTC) Date: Wed, 14 Jun 2023 07:59:04 +0000 (UTC) From: Richard Biener To: Jiufu Guo cc: Segher Boessenkool , gcc-patches@gcc.gnu.org, dje.gcc@gmail.com, linkw@gcc.gnu.org, bergner@linux.ibm.com, richard.sandiford@arm.com, jeffreyalaw@gmail.com Subject: Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie In-Reply-To: <7no7liadru.fsf@ltcden2-lp1.aus.stglabs.ibm.com> Message-ID: References: <20230613122335.2108620-1-guojiufu@linux.ibm.com> <20230613183320.GU19790@gate.crashing.org> <7no7liadru.fsf@ltcden2-lp1.aus.stglabs.ibm.com> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wed, 14 Jun 2023, Jiufu Guo wrote: > > Hi, > > Segher Boessenkool writes: > > > Hi! > > > > As I said in a reply to the original patch: not okay. Sorry. > > Thanks a lot for your comments! > I'm also thinking about other solutions: > 1. "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])" > This is the existing pattern. It may be read as an action > to clean an unknown-size memory block. > > 2. "set (mem/c:BLK (reg/f:DI 1 1) unspec:blk (const_int 0 [0]) > UNSPEC_TIE". > Current patch is using this one. > > 3. "set (mem/c:DI (reg/f:DI 1 1) unspec:DI (const_int 0 [0]) > UNSPEC_TIE". > This avoids using BLK on unspec, but using DI. That gives the MEM a size which means we can interpret the (set ..) as killing a specific area of memory, enabling DSE of earlier stores. AFAIU this special instruction is only supposed to prevent code motion (of stack memory accesses?) across this instruction? I'd say a (may_clobber (mem:BLK (reg:DI 1 1))) might be more to the point? I've used "may_clobber" which doesn't exist since I'm not sure whether a clobber is considered a kill. The docs say "Represents the storing or possible storing of an unpredictable..." - what is it? Storing or possible storing? I suppose stack_tie should be less strict than the documented (clobber (mem:BLK (const_int 0))) (clobber all memory). ? > 4. "set (mem/c:BLK (reg/f:DI 1 1) unspec (const_int 0 [0]) > UNSPEC_TIE" > There is still a mode for the unspec. > > > > > > But some comments on this patch: > > > > On Tue, Jun 13, 2023 at 08:23:35PM +0800, Jiufu Guo wrote: > >> + && XINT (SET_SRC (set), 1) == UNSPEC_TIE > >> + && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx); > > > > This makes it required that the operand of an UNSPEC_TIE unspec is a > > const_int 0. This should be documented somewhere. Ideally you would > > want no operand at all here, but every unspec has an operand. > > Right! Since checked UNSPEC_TIE arleady, we may not need to check > the inner operand. Like " && XINT (SET_SRC (set), 1) == UNSPEC_TIE);". > > > > >> + RTVEC_ELT (p, i) > >> + = gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), > >> + UNSPEC_TIE)); > > > > If it is hard to indent your code, your code is trying to do to much. > > Just have an extra temporary? > > > > rtx un = gen_rtx_UNSPEC (BLKmode, gen_rtvec (1, const0_rtx), UNSPEC_TIE); > > RTVEC_ELT (p, i) = gen_rtx_SET (mem, un); > > > > That is shorter even, and certainly more readable :-) > > Yeap, thanks! > > > > >> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block" > >> operands[4] = gen_frame_mem (Pmode, operands[1]); > >> p = rtvec_alloc (1); > >> RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]), > >> - const0_rtx); > >> + gen_rtx_UNSPEC (BLKmode, > >> + gen_rtvec (1, const0_rtx), > >> + UNSPEC_TIE)); > >> operands[5] = gen_rtx_PARALLEL (VOIDmode, p); > > > > I have a hard time to see how this could ever be seen as clearer or more > > obvious or anything like that :-( > > I was thinking about just invoking gen_stack_tie here. > > BR, > Jeff (Jiufu Guo) > > > > > > > Segher > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)