From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.223.131]) by sourceware.org (Postfix) with ESMTPS id EB6613858C39 for ; Tue, 28 Nov 2023 07:50:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EB6613858C39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EB6613858C39 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=195.135.223.131 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701157844; cv=none; b=E1lk+4l38aNsVEXYOV1sLVMT7kl5U05JvZ73ChZL4RBWMDRaRoyFs1a2//y3GxbzexGCvECEmngabSqJUo3Y2m0wSmVIejMd4tUX8BiaiC5hkfVkYmm7pjJB6DPZWOFnylQ/L9akmHCIXZ4/zo8HomosoGeMnURX3IvRG9JjSjc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701157844; c=relaxed/simple; bh=/Jkq0+1M9JipCCpCOSlWHOWRb5NnCt186h2zj0BwGxI=; h=Date:From:To:Subject:Message-ID:MIME-Version; b=pVhIW1MphB8Zut+Q1OGvwgBLwM9S2J6icAjUbtwG1tYsNOIwpq5Ihm0spLF+67tTY5S0DvressjthfwaZF20WUu9cGA8i4NDQjCqvwRB5T/0CYZuR3Tnwm/mHbz6Z0GfrOZMQVuzZyjB6XOeunT0SRW6d6MUengG97Igf6xo/N4= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from relay2.suse.de (unknown [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id B2DAA1F459; Tue, 28 Nov 2023 07:50:40 +0000 (UTC) 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 370F52C15A; Tue, 28 Nov 2023 07:50:39 +0000 (UTC) Date: Tue, 28 Nov 2023 07:50:39 +0000 (UTC) From: Richard Biener To: Jeff Law cc: Robin Dapp , gcc-patches@gcc.gnu.org, "Li, Pan2" Subject: Re: [PATCH][RFC] middle-end/110237 - wrong MEM_ATTRs for partial loads/stores In-Reply-To: Message-ID: References: <20230621075019.7CA813858033@sourceware.org> <5b04dad7-a241-4ad3-b4a5-b81080b67e90@gmail.com> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Level: X-Rspamd-Server: rspamd1 Authentication-Results: smtp-out2.suse.de; none X-Rspamd-Queue-Id: B2DAA1F459 X-Spam-Score: -4.00 X-Spamd-Result: default: False [-4.00 / 50.00]; TAGGED_RCPT(0.00)[]; REPLY(-4.00)[] X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00,KAM_DMARC_STATUS,KAM_SHORT,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, 27 Nov 2023, Jeff Law wrote: > > > On 11/27/23 05:39, Robin Dapp wrote: > >> 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 riscv we're seeing a similar problem across the testsuite > > and several execution failures as a result. In the case I > > looked at we move a scalar load upwards over a partial store > > that aliases the load. > > > > I independently arrived at the spot mentioned in > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110237#c4 > > before knowing about the PR. > > > > I can confirm that your RFC patch fixes at least two of the > > failures, I haven't checked the others but very likely > > they are similar. > FWIW, it should always be safe to ignore the memory attributes. So if > there's a reasonable condition here, then we can use it and just ignore the > attribute. > > Does the attribute on a partial load/store indicate the potential load/store > size or does it indicate the actual known load/store size. If the former, then > we probably need to treat it as a may-read/may-write kind of reference. There's no way to distinguish a partial vs. non-partial MEM on RTL and while without the bogus MEM_ATTR the alias oracle pieces that miscompiled the original case are fended off we still see the load/store as full given they have a mode with a size - that for example means that DSE can elide a previous store to a masked part. Eventually that's fended off by using an UNSPEC, but whether the RTL IL has the correct semantics is questionable. That said, I did propose scrapping the MEM_EXPR which I think is the correct thing to do unless we want to put a CALL_EXPR into it (nothing would use that at the moment) or re-do MEM_EXPR and instead have an ao_ref (or sth slightly more complete) instead of the current MEM_ATTRs - but that would be a lot of work. This leaves the question wrt. semantics of for example x86 mask_store: (insn 23 22 24 5 (set (mem:V4DF (plus:DI (reg/v/f:DI 106 [ x ]) (reg:DI 101 [ ivtmp.15 ])) [2 MEM [(double *)x_11(D) + ivtmp.15_33 * 1]+0 S32 A64]) (unspec:V4DF [ (reg:V4DI 104 [ mask__16.8 ]) (reg:V4DF 105 [ vect_cst__42 ]) (mem:V4DF (plus:DI (reg/v/f:DI 106 [ x ]) (reg:DI 101 [ ivtmp.15 ])) [2 MEM [(double *)x_11(D) + ivtmp.15_33 * 1]+0 S32 A64]) ] UNSPEC_MASKMOV)) "t.c":5:12 8523 {avx_maskstorepd256} (nil)) it uses a read-modify-write which makes it safe for DSE. mask_load looks like (insn 28 27 29 6 (set (reg:V4DF 115 [ vect__7.11 ]) (unspec:V4DF [ (reg:V4DI 114 [ mask__8.8 ]) (mem:V4DF (plus:DI (reg/v/f:DI 118 [ val ]) (reg:DI 103 [ ivtmp.29 ])) [2 MEM [(double *)val_13(D) + ivtmp.29_22 * 1]+0 S32 A64]) ] UNSPEC_MASKMOV)) "t.c":5:17 8515 {avx_maskloadpd256} (nil)) both have (as operand of the UNSPEC) a MEM with V4DFmode (and a MEM_EXPR with a similarly bougs MEM_EXPR) indicating the loads are _not_ partial. That means the disambiguation against a store to an object that's smaller than V4DF is still possible. Setting MEM_SIZE to UNKNOWN doesn't help - that just asks to look at the mode. As discussed using a BLKmode MEM _might_ be a way out but I didn't try what will happen then (patterns would need to be adjusted I guess). That said, I'm happy to commit the partial fix, scrapping the bogus MEM_EXPRs. OK for that? Thanks, Richard.