From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 2E345384AB58 for ; Fri, 3 May 2024 17:30:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2E345384AB58 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 2E345384AB58 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714757454; cv=none; b=odgfYgW5aoTGkG2uScuioIuchGktEJPnwo3iIwFDK/OgCT8kNvRJznrtQOOT6KkbuL57fQRojPPiIBRcIXQtfr5lz2uQmLtCPYWh/EKbtuCaqd6VlTu+PrjQrl4ENwKt7FJ5UUnEsUD+EVTNbQLGFzbTxxNBIAxIlifKBPtRZRI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1714757454; c=relaxed/simple; bh=t9FHeEnO7C6PY3gkRjyP0V/d9MHgFu9t/6kJb0Q8JzI=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=n8OLwn7tvcf7/sCexh31oZql/MMW/1eSTAQWuc4sQvLM7Kf/VGtjj8TPmGp977cdflPEINVeZDb6FUAQEG6lu8x+3rwdySc4KZ7alX6wsvvUhcatT7ujS36V6djdQ/8MJjvPI3173sEZLFytOqrPwj0yg099QES8KjfGFTNVDRA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1714757451; h=from:from:reply-to:reply-to:subject:subject: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=79V/7LTpzCVnCZrO5okCqcA1foxPJbKLB1jGGrEI1yE=; b=jF+qI2V2ntsR0wi0sm7unpnddJ/jKUGLVsbyvyen7JnqX2KX3SzaHyMMT2D4frcpjXNTlW NoK7rAlTebCHeeH/X64H4uvxjzzrPW3Yuo/H5CYbBWz+vrVQoKpsFkqundESNo1mfXFhmn Q93SvJmX5H8ctzHlApa1YAhbyMEU30E= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-307-vZUXxiJ0MtyCXsQerv-SxA-1; Fri, 03 May 2024 13:30:50 -0400 X-MC-Unique: vZUXxiJ0MtyCXsQerv-SxA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D1C1480017B; Fri, 3 May 2024 17:30:49 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.45.224.64]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7C5DE40C6CC0; Fri, 3 May 2024 17:30:49 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 443HUlpC3198118 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Fri, 3 May 2024 19:30:47 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 443HUlfC3198117; Fri, 3 May 2024 19:30:47 +0200 Date: Fri, 3 May 2024 19:30:46 +0200 From: Jakub Jelinek To: Martin Uecker Cc: Richard Biener , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] middle-end/114931 - type_hash_canon and structual equality types Message-ID: Reply-To: Jakub Jelinek References: <20240503121400.C8A4313991@imap1.dmz-prg2.suse.org> <3616afcc455c1f80e2bb8a80a408994a67062d85.camel@tugraz.at> MIME-Version: 1.0 In-Reply-To: <3616afcc455c1f80e2bb8a80a408994a67062d85.camel@tugraz.at> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP 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 Fri, May 03, 2024 at 05:32:12PM +0200, Martin Uecker wrote: > Am Freitag, dem 03.05.2024 um 14:13 +0200 schrieb Richard Biener: > > TYPE_STRUCTURAL_EQUALITY_P is part of our type system so we have > > to make sure to include that into the type unification done via > > type_hash_canon. This requires the flag to be set before querying > > the hash which is the biggest part of the patch. > > I assume this does not affect structs / unions because they > do not make this mechanism of type unification (each tagged type > is a unique type), but only derived types that end up having > TYPE_STRUCTURAL_EQUALITY_P because they are constructed from > incomplete structs / unions before TYPE_CANONICAL is set. > > I do not yet understand why this change is needed. Type > identity should not be affected by setting TYPE_CANONICAL, so > why do we need to keep such types separate? I understand that we > created some inconsistencies, but I do not see why this change > is needed to fix it. But I also haven't understood how we ended > up with a TYPE_CANONICAL having TYPE_STRUCTURAL_EQUALITY_P in > PR 114931 ... So, the C23 situation before the r14-10045 change (not counting the r14-9763 change that was later reverted) was that sometimes TYPE_CANONICAL of a RECORD_TYPE/UNION_TYPE could change from self to a different RECORD_TYPE/UNION_TYPE and we didn't bother to adjust derived types. That was really dangerous, I think e.g. type alias set wasn't recomputed. r14-10045 changed it to the non-ideal, but perhaps less wrong model, where we start with TYPE_STRUCTURAL_EQUALITY_P on incomplete types in C23 and perhaps later on change them to !TYPE_STRUCTURAL_EQUALITY_P when the type is completed, and adjust TYPE_CANONICAL of some easily discoverable derived types but certainly not all. Still, that change introduces something novel to the type system, namely that TYPE_CANONICAL can change on a type, even when it is just limited to the TYPE_STRUCTURAL_EQUALITY_P -> !TYPE_STRUCTURAL_EQUALITY_P kind of change and we never change one non-NULL TYPE_CANONICAL to a different one (ok, not counting the short lived TYPE_CANONICAL being initially set to self during make_node and then quickly adjusted in the callers). One question is, could we for C23 somehow limit this for the most common case where people just forward declare some aggregate type and then soon after define it? But guess the problematic counterexample there is struct S; // forward declare struct S *p; // create some derived types from it void foo (void) { struct S { int s; }; // define the type in a different scope // (perhaps with a forward declaration as well) struct S s; use (&s); // create derived types } struct S { int s; }; // define the type in the global scope to something // that matches previously defined struct S in // another scope So e.g. noting in the hash table that a particular type has been forward declared so far and using TYPE_STRUCTURAL_EQUALITY_P only if it has been forward declared in some other scope wouldn't work. Another question is whether c_update_type_canonical can or should try to update TYPE_ALIAS_SET if TYPE_ALIAS_SET_KNOWN_P. Or do we never cache alias sets for TYPE_STRUCTURAL_EQUALITY_P types? Anyway, the ICE on the testcase is because alias.cc asserts that a !TYPE_STRUCTURAL_EQUALITY_P (type) has !TYPE_STRUCTURAL_EQUALITY_P (TYPE_CANONICAL (type)). The possibilities to resolve that are either at c_update_type_canonical time try to find all the derived types rather than just some and recompute their TYPE_CANONICAL. Guess we could e.g. just traverse the whole type_hash_table hash table and for each type see if it is in any way related to the type that is being changed and then recompute them. Though, especially FUNCTION_TYPEs make that really ugly and furthermore it needs to be recomputed in the right order, basically in the derivation order. Without doing that, we'll have some TYPE_STRUCTURAL_EQUALITY_P derived types in the type_hash_table hash table; that is conservatively correct, but can result in worse code generation because of using alias set 0. Another possibility is what Richi posted, essentially stop reusing derived types created from the time when the base type was incomplete when asking for a new derived type. We'll get the TYPE_STRUCTURAL_EQUALITY_P derived types if they were created before the base type was completed when used directly (e.g. when it is a TREE_TYPE of some decl etc.), but when we ask for a new type we'd disregard the old type and create a new one. I'm not sure the patch is complete for that, because it doesn't adjust check_base_type, build_pointer_type_for_mode, build_reference_type_for_mode which don't use type_hash_canon but walk TYPE_NEXT_VARIANT list or TYPE_POINTER_TO or TYPE_REFERENCE_TO chains. Though, maybe it is ok as is when c_update_type_canonical adjusts the pointer types and variant types, those will be adjusted and as soon as we hit the first ARRAY_TYPE/FUNCTION_TYPE or other type_canon_hash using derived types and they aren't shared but have a new one created, further derived pointer/qualified types will be based on the new ones, not the old ones. Another possibility is what I wrote in the PR, ensure the !TYPE_STRUCTURAL_EQUALITY_P (type) -> !TYPE_STRUCTURAL_EQUALITY_P (TYPE_CANONICAL (type)) implication alias.cc relies on is held. Without the complete update of all derived types or Richi's patch, there is always a chance that some TYPE_CANONICAL (...) = assignment will get a TYPE_STRUCTURAL_EQUALITY_P type which was created while the base type was incomplete. So, we could just check for that case and use conservative type as well. I.e. after each TYPE_CANONICAL (...) = ... (except the TYPE_CANONICAL (t) = t; cases which are always fine) add if (TYPE_STRUCTURAL_EQUALITY_P (TYPE_CANONICAL (type))) SET_TYPE_STRUCTURAL_EQUALITY (type); That is conservatively fine. And yet another option is to change alias.cc to handle the TYPE_STRUCTURAL_EQUALITY_P (TYPE_CANONICAL (type)) case the same as TYPE_STRUCTURAL_EQUALITY_P (type) instead of asserting it is not ok. I think we'd get best generated code if we managed to update all derived types, Richi's patch is the second and the other changes result in even worse code. Jakub