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 01C733858D20 for ; Mon, 26 Jun 2023 08:42:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 01C733858D20 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 F0C1E218BB; Mon, 26 Jun 2023 08:41:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1687768919; 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=3tAEd3wkhTEFB3LeQPYEVBFBXFwBYB+YpJBeyUsqtnY=; b=Lm3wwcMCLUPr2VtEz0Zy/5rd2pthHO+Fgy+v7m7cN6VlxF3aXsUUlDm++27cm9GZSVlNBu wH51XzwGMl9vG0UQSQp4iYVWcbTbbKkFZP15jO8UrR3nV5pYNf5O+owjHU/3rFpeOw5aYw btd++WAydfiFC8evP8of1RtkZIowRs0= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1687768919; 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=3tAEd3wkhTEFB3LeQPYEVBFBXFwBYB+YpJBeyUsqtnY=; b=sMl58HbHjyxl/1I1J1OHuOHsk/ia7FV6qDB/MK6FWT+EtPcFMXjbNBX8qx20AmY+LHmguT IdOYVfHrw5ZbpIDg== 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 E49C82C146; Mon, 26 Jun 2023 08:41:59 +0000 (UTC) Date: Mon, 26 Jun 2023 08:41:59 +0000 (UTC) From: Richard Biener To: Hongtao Liu cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH][RFC] middle-end/110237 - wrong MEM_ATTRs for partial loads/stores In-Reply-To: Message-ID: References: <20230621074956.1174B3858288@sourceware.org> 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=-10.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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 Mon, 26 Jun 2023, Hongtao Liu wrote: > On Wed, Jun 21, 2023 at 3:49?PM Richard Biener via Gcc-patches > wrote: > > > > The following addresses a miscompilation by RTL scheduling related > > to the representation of masked stores. For that we have > > > > (insn 38 35 39 3 (set (mem:V16SI (plus:DI (reg:DI 40 r12 [orig:90 _22 ] [90]) > > (const:DI (plus:DI (symbol_ref:DI ("b") [flags 0x2] ) > > (const_int -4 [0xfffffffffffffffc])))) [1 MEM [(int *)vectp_b.12_28]+0 S64 A32]) > > (vec_merge:V16SI (reg:V16SI 20 xmm0 [118]) > > (mem:V16SI (plus:DI (reg:DI 40 r12 [orig:90 _22 ] [90]) > > (const:DI (plus:DI (symbol_ref:DI ("b") [flags 0x2] ) > > (const_int -4 [0xfffffffffffffffc])))) [1 MEM [(int *)vectp_b.12_28]+0 S64 A32]) > > > > and specifically the memory attributes > > > > [1 MEM [(int *)vectp_b.12_28]+0 S64 A32] > > > > are problematic. They tell us the instruction stores and reads a full > > vector which it if course does not. There isn't any good MEM_EXPR > > we can use here (we lack a way to just specify a pointer and restrict > > info for example), and since the MEMs have a vector mode it's > > difficult in general as passes do not need to look at the memory > > attributes at all. > > > > The easiest way to avoid running into the alias analysis problem is > > to scrap the MEM_EXPR when we expand the internal functions for > > partial loads/stores. That avoids the disambiguation we run into > > which is realizing that we store to an object of less size as > > the size of the mode we appear to store. > > > > After the patch we see just > > > > [1 S64 A32] > > > > so we preserve the alias set, the alignment and the size (the size > > is redundant if the MEM insn't BLKmode). That's still not good > > in case the RTL alias oracle would implement the same > > disambiguation but it fends off the gimple one. > > > > This fixes gcc.dg/torture/pr58955-2.c when built with AVX512 > > and --param=vect-partial-vector-usage=1. > > > > On the MEM_EXPR side we could use a CALL_EXPR and on the RTL > > side we might instead want to use a BLKmode MEM? Any better > > ideas here? > Can we introduce a new member in class mem_attrs and ao_ref, initial > the member (named partial_access_p) in expand_partial_load_optab_fn > and expand_partial_store_optab_fn, pass partial_access_p from > mem_attrs to ao_ref in djust ao_ref_from_mem. > It looks to me we only want to avoid the below rule in alias analysis. > For others, size, max_size, offset is still meaningful, even for > rtx_addr_can_trap_p, if size can't trap, partial access must not trap? The GIMPLE oracle makes sure to set 'size' to -1 (not known) when seeing the IFNs for masked stores/loads. To fix the RTL MEM_EXPR side I would rather try putting a CALL_EXPR there, preserving the masked internal function, instead of using a mem-ref and additional info. That leaves the MEM RTX itself - for a MEM with non-BLKmode I thin MEM_ATTRS are completely optional and it's OK to drop them, we'd have to special case the partial MEM RTX then. Jeff agreed that eventually using BLKmode for them would be "OK", then we can make MEM_SIZE unknown as well. I think the issue is latent on branches so I first wanted to find some minimal change to mitigate the miscompile and then maybe try options to not lose most of the alias info here. Richard. > /* If the pointer based access is bigger than the variable they cannot > alias. This is similar to the check below where we use TBAA to > increase the size of the pointer based access based on the dynamic > type of a containing object we can infer from it. */ > poly_int64 dsize2; > if (known_size_p (size1) --- should be unknown?? > && poly_int_tree_p (DECL_SIZE (base2), &dsize2) > && known_lt (dsize2, size1)) > return false; > > > > > Thanks, > > Richard. > > > > PR middle-end/110237 > > * internal-fn.cc (expand_partial_load_optab_fn): Clear > > MEM_EXPR and MEM_OFFSET. > > (expand_partial_store_optab_fn): Likewise. > > --- > > gcc/internal-fn.cc | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/gcc/internal-fn.cc b/gcc/internal-fn.cc > > index c911ae790cb..2dc685e7d85 100644 > > --- a/gcc/internal-fn.cc > > +++ b/gcc/internal-fn.cc > > @@ -2903,6 +2903,10 @@ expand_partial_load_optab_fn (internal_fn, gcall *stmt, convert_optab optab) > > > > mem = expand_expr (rhs, NULL_RTX, VOIDmode, EXPAND_WRITE); > > gcc_assert (MEM_P (mem)); > > + /* The built MEM_REF does not accurately reflect that the load > > + is only partial. Clear it. */ > > + set_mem_expr (mem, NULL_TREE); > > + clear_mem_offset (mem); > > mask = expand_normal (maskt); > > target = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE); > > create_output_operand (&ops[0], target, TYPE_MODE (type)); > > @@ -2971,6 +2975,10 @@ expand_partial_store_optab_fn (internal_fn, gcall *stmt, convert_optab optab) > > > > mem = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE); > > gcc_assert (MEM_P (mem)); > > + /* The built MEM_REF does not accurately reflect that the store > > + is only partial. Clear it. */ > > + set_mem_expr (mem, NULL_TREE); > > + clear_mem_offset (mem); > > mask = expand_normal (maskt); > > reg = expand_normal (rhs); > > create_fixed_operand (&ops[0], mem); > > -- > > 2.35.3 > > > > -- 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)