From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id E296D396ECE5 for ; Thu, 10 Sep 2020 09:09:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E296D396ECE5 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rguenther@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 611EAAC68; Thu, 10 Sep 2020 09:09:58 +0000 (UTC) Date: Thu, 10 Sep 2020 11:09:42 +0200 (CEST) From: Richard Biener To: Kito Cheng cc: GCC Patches , Kito Cheng , ebotcazou@adacore.com Subject: Re: [PATCH] PR target/96759 - Handle global variable assignment from misaligned structure/PARALLEL return values. In-Reply-To: Message-ID: References: <20200910034405.39647-1-kito.cheng@sifive.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Thu, 10 Sep 2020 09:09:45 -0000 On Thu, 10 Sep 2020, Kito Cheng wrote: > Hi Richard: > > Thanks for your review :) > > > riscv doesn't seem to have movmisalign and I don't see why movmisalign > > should not support a TImode parallel RHS so at least you should > > put this check after the icode != CODE_FOR_nothing check? > > RISC-V has an option `-mno-strict-align` to enable mis-aligned access, > but we didn't provide movmisalign pattern, I guess might be another issue, > I tried to add movmisalign pattern and then it will feed PARALLEL into that, > and got into a similar situation again, most targets are not except > got PARALLEL, > so that's the reason why I put before icode != CODE_FOR_nothing check. > > > Also wouldn't it be better to support PARALLEL from within > > store_bit_field? > For the above reason, I think store_bit_field supports PARALLEL is not enough. > > > After all this is a misaligned access and > > since you didn't quote the RTL involved I'm guessing that > > emit_group_store knows nothing about this fact. > > Oh, good point, how do you think about loading into temp and then > storing it in the DECL, so that we could use original logic to > handle misaligned store. > > A PoC for this idea, didn't fully tested yet: > > diff --git a/gcc/expr.c b/gcc/expr.c > index 1a15f24b3979..6ef97740c764 100644 > --- a/gcc/expr.c > +++ b/gcc/expr.c > @@ -5166,8 +5166,19 @@ expand_assignment (tree to, tree from, bool nontemporal) > || targetm.slow_unaligned_access (mode, align))) > { > rtx reg, mem; > + push_temp_slots (); > > reg = expand_expr (from, NULL_RTX, VOIDmode, EXPAND_NORMAL); I'd say there must be (wishful thinking) some expand_expr modifier that guarantees this? Alternatively use reg = force_reg (mode, reg); instead of assign_stack_temp & emit_group_store? That said, I don't know very much about the fancy ways to handle stores from PARALLEL - but doesn't PARALLEL mean we can just take any of its members as source? How does 'reg' look like exactly in your case? What does 'to' expand to? Richard. > + > + if (GET_CODE (reg) == PARALLEL) > + { > + rtx temp = assign_stack_temp (mode, > + GET_MODE_SIZE (mode)); > + emit_group_store (temp, reg, TREE_TYPE (from), > + int_size_in_bytes (TREE_TYPE (from))); > + reg = temp; > + } > + > reg = force_not_mem (reg); > mem = expand_expr (to, NULL_RTX, VOIDmode, EXPAND_WRITE); > if (TREE_CODE (to) == MEM_REF && REF_REVERSE_STORAGE_ORDER (to)) > @@ -5186,6 +5197,7 @@ expand_assignment (tree to, tree from, bool nontemporal) > else > store_bit_field (mem, GET_MODE_BITSIZE (mode), 0, 0, 0, mode, reg, > false); > + pop_temp_slots (); > return; > } > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)