From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2a07:de40:b251:101:10:150:64:1]) by sourceware.org (Postfix) with ESMTPS id 0B30C3858D28 for ; Tue, 20 Feb 2024 08:01:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0B30C3858D28 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 0B30C3858D28 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a07:de40:b251:101:10:150:64:1 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708416078; cv=none; b=nIJk8yhxmCnmLCCJS84WN7i8sBgPjGwdS5ZIXb5lntu4D7p8PPsu8gzoTplE1ybrxoIlqpelOt7Yc1CQGKC6AIq+fGX/MFtlDts6wIdNfJduL7gDg4AqwmDK7sMARFXANtFEzHv8CeoWlfp68oHbgcz/+PvQ2pSXw6G/cxKMl7I= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708416078; c=relaxed/simple; bh=lmSUR7hVDZzPyodoVwwgySbniy+pMPra30LU7OLoMSk=; h=DKIM-Signature:DKIM-Signature:DKIM-Signature:DKIM-Signature:Date: From:To:Subject:Message-ID:MIME-Version; b=dzm3G34JLe2jNBtD3tJsSHn0oF6gFUOqbyNJuN4anxT5f+YpMRh6PC4NZc5pzjnvGcOe/Wl/BtYsvax3/dFgBHJ+YEIAXvPA89zdiA1GGritqNFwLkOCoJQwSj2Fk5lLkSUIWG/MWQfTyRb4JqcK7qVMliIpnJSWoxSxcHUVIEM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from [10.168.4.150] (unknown [10.168.4.150]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id BF7F021F61; Tue, 20 Feb 2024 08:01:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1708416070; 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=X/Ca6lJkSJnZmgNTeq5ZDwHQFW706W2kmiVy6/qD3tA=; b=lyIo0WqoD2ASwKrVL9c37H4xCAvrZSM4ospHU4TPny2bYyg4+I8+UdF3gW6uw9Sc/z7LjS qbKGiOBHxza0Oig6UJgv+5O8C6tpxEHNH5N4F2om7UHip40RnD4SLmCyPPqsPIe1fqEquP T7KRywrcguPPeuM+OhREGGROk+VhqtA= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1708416070; 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=X/Ca6lJkSJnZmgNTeq5ZDwHQFW706W2kmiVy6/qD3tA=; b=tB25bqNkh1+Qn+tSgEemIbOxV5m2BK5eInhWJJgfC1G1ohXqZNEMgDYqkOB28rwwQuYAE2 FogIcAzmzHxlWMCw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1708416070; 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=X/Ca6lJkSJnZmgNTeq5ZDwHQFW706W2kmiVy6/qD3tA=; b=lyIo0WqoD2ASwKrVL9c37H4xCAvrZSM4ospHU4TPny2bYyg4+I8+UdF3gW6uw9Sc/z7LjS qbKGiOBHxza0Oig6UJgv+5O8C6tpxEHNH5N4F2om7UHip40RnD4SLmCyPPqsPIe1fqEquP T7KRywrcguPPeuM+OhREGGROk+VhqtA= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1708416070; 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=X/Ca6lJkSJnZmgNTeq5ZDwHQFW706W2kmiVy6/qD3tA=; b=tB25bqNkh1+Qn+tSgEemIbOxV5m2BK5eInhWJJgfC1G1ohXqZNEMgDYqkOB28rwwQuYAE2 FogIcAzmzHxlWMCw== Date: Tue, 20 Feb 2024 09:01:10 +0100 (CET) From: Richard Biener To: Jakub Jelinek cc: Jason Merrill , gcc-patches@gcc.gnu.org, Jonathan Wakely Subject: Re: [PATCH] c-family, c++: Fix up handling of types which may have padding in __atomic_{compare_}exchange In-Reply-To: Message-ID: <39q9r137-3884-67oq-2334-70q42so2sp22@fhfr.qr> References: <5988812a-3f21-4ef4-9205-fe267c356d58@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Authentication-Results: smtp-out1.suse.de; none X-Spamd-Result: default: False [-3.10 / 50.00]; ARC_NA(0.00)[]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; RCVD_COUNT_ZERO(0.00)[0]; FUZZY_BLOCKED(0.00)[rspamd.com]; BAYES_HAM(-3.00)[100.00%] X-Spam-Level: X-Spam-Score: -3.10 X-Spam-Status: No, score=-5.0 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 Tue, 20 Feb 2024, Jakub Jelinek wrote: > On Tue, Feb 20, 2024 at 12:12:11AM +0000, Jason Merrill wrote: > > On 2/19/24 02:55, Jakub Jelinek wrote: > > > On Fri, Feb 16, 2024 at 01:51:54PM +0000, Jonathan Wakely wrote: > > > > Ah, although __atomic_compare_exchange only takes pointers, the > > > > compiler replaces that with a call to __atomic_compare_exchange_n > > > > which takes the newval by value, which presumably uses an 80-bit FP > > > > register and so the padding bits become indeterminate again. > > > > > > The problem is that __atomic_{,compare_}exchange lowering if it has > > > a supported atomic 1/2/4/8/16 size emits code like: > > > _3 = *p2; > > > _4 = VIEW_CONVERT_EXPR (_3); > > > > Hmm, I find that surprising; I thought of VIEW_CONVERT_EXPR as working on > > lvalues, not rvalues, based on the documentation describing it as roughly > > *(type2 *)&X. > > It works on both, if it is on the lhs, it obviously needs to be on an lvalue > and is something like > VIEW_CONVERT_EXPR (mem) = something; > and if it is on rhs, it can be on an rvalue, just reinterpret bits of > something as something else, so more like > ((union { typeof (val) x; I_type y; }) (val)).y > > > Now I see that gimplify_expr does what you describe, and I wonder what the > > advantage of that is. That seems to go back to richi's r206420 for PR59471. > > r270579 for PR limited it to cases where the caller wants an rvalue; perhaps > > it should also be avoided when the operand is an INDIRECT_REF? > > Strangely we don't even try to optimize it, even at -O2 that > _3 = *p2_2(D); > _4 = VIEW_CONVERT_EXPR (_3); > stays around until optimized. I believe VIEW_CONVERT_EXPR is valid > around a memory reference, so it could be either > _4 = VIEW_CONVERT_EXPR (*p2_2(D)); > or the MEM_REF with aliasing info from original pointer and type from VCE. > For optimizations, guess it is a matter of writing some match.pd rule, but > we can't rely on it for the atomics. I'm not sure those would be really equivalent (MEM_REF vs. V_C_E as well as combined vs. split). It really depends how RTL expansion handles this (as you can see padding can be fun here). So I'd be nervous for a match.pd rule here (also we can't match memory defs). As for your patch I'd go with a MEM_REF unconditionally, I don't think we want different behavior whether there's padding or not ... > Doing it in the gimplifier is possible, but not sure how to do that easily, > given that the VIEW_CONVERT_EXPR argument can be either lvalue or rvalue > and we need to gimplify it first. > The first part is exactly what forces it into a separate SSA_NAME for the > load vs. VIEW_CONVERT_EXPR around it: > > case VIEW_CONVERT_EXPR: > if ((fallback & fb_rvalue) > && is_gimple_reg_type (TREE_TYPE (*expr_p)) > && is_gimple_reg_type (TREE_TYPE (TREE_OPERAND (*expr_p, 0)))) > { > ret = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, > post_p, is_gimple_val, fb_rvalue); > recalculate_side_effects (*expr_p); > break; > } that was done to help optimizing some cases (IIRC with vectors / int punning). > /* Fallthru. */ > > case ARRAY_REF: > case ARRAY_RANGE_REF: > case REALPART_EXPR: > case IMAGPART_EXPR: > case COMPONENT_REF: > ret = gimplify_compound_lval (expr_p, pre_p, post_p, > fallback ? fallback : fb_rvalue); > break; > > but if we do the gimplify_compound_lval, we'd actually force it to be > addressable (with possible errors if that isn't possible) etc. > Having just a special-case of VIEW_CONVERT_EXPR of INDIRECT_REF and > do gimplify_compound_lval in that case mikght be wrong I think if > e.g. INDIRECT_REF operand is ADDR_EXPR, shouldn't we cancel *& in that case > and still not force it into memory? > > The INDIRECT_REF: case is: > { > bool volatilep = TREE_THIS_VOLATILE (*expr_p); > bool notrap = TREE_THIS_NOTRAP (*expr_p); > tree saved_ptr_type = TREE_TYPE (TREE_OPERAND (*expr_p, 0)); > > *expr_p = fold_indirect_ref_loc (input_location, *expr_p); > if (*expr_p != save_expr) > { > ret = GS_OK; > break; > } > > ret = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p, > is_gimple_reg, fb_rvalue); > if (ret == GS_ERROR) > break; > > recalculate_side_effects (*expr_p); > *expr_p = fold_build2_loc (input_location, MEM_REF, > TREE_TYPE (*expr_p), > TREE_OPERAND (*expr_p, 0), > build_int_cst (saved_ptr_type, 0)); > TREE_THIS_VOLATILE (*expr_p) = volatilep; > TREE_THIS_NOTRAP (*expr_p) = notrap; > ret = GS_OK; > break; > } > so I think if we want to special case VIEW_CONVERT_EXPR on INDIRECT_REF, > we should basically copy and adjust that to the start of the case > VIEW_CONVERT_EXPR:. In particular, if fold_indirect_ref_loc did something > and it isn't a different INDIRECT_REF or something addressable, do what it > does now (i.e. possibly treat as lvalue), otherwise gimplify the INDIRECT_REF > operand and build a MEM_REF, but with the type of the VIEW_CONVERT_EXPR but > still saved_ptr_type of the INDIRECT_REF. > > Though, that all still feels like an optimization rather than guarantee > which is what we need for the atomics. Yes, and I think the frontend does it wrong - if it wants to do a load of l_type it should do that, not re-interpret the result. I suppose the docs @defbuiltin{void __atomic_exchange (@var{type} *@var{ptr}, @var{type} *@var{val}, @var{type} *@var{ret}, int @var{memorder})} This is the generic version of an atomic exchange. It stores the contents of @code{*@var{val}} into @code{*@var{ptr}}. The original value of @code{*@var{ptr}} is copied into @code{*@var{ret}}. contain too little standards legalise to constrain 'type', it just appears to use lvalues of *ptr, *val and *ret here which for floats with padding possibly isn't well-defined and "contents" isn't a standard term (it doesn't say bit representation or something similar), it also says "stores" and "copied into". That said, if the FE determines in l_type a type suitable for copying then the access should use a MEM_REF, no further "conversion" required. Richard.