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 397EC3858D1E for ; Tue, 24 Jan 2023 10:06:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 397EC3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1674554760; 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=vZX1mviga3nMAr7SyxJiqkY+eZNarjBW8LpNAX1aUfQ=; b=MeqoTeR+8R8vmXs+mTjSe+CWv9j8ATBHOtpbWTI5LauqurWeFAuKMtuepzL0LMbIsOKjVx e3O0wH3NO85SfLSZISeKoZc/6XzhteXNmS340gr/QmdgvTudM4V4twrvT2XwaZQmS1MYtF yYBVtL01N+7iNHxYi6Zi3pWGrxlbQKQ= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-563-TZyusqeiMnWoAQen4KHSNA-1; Tue, 24 Jan 2023 05:05:59 -0500 X-MC-Unique: TZyusqeiMnWoAQen4KHSNA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 5B6E718E6C41; Tue, 24 Jan 2023 10:05:59 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.223]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 18FC0140EBF5; Tue, 24 Jan 2023 10:05:58 +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 30OA5uJe466258 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 24 Jan 2023 11:05:56 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 30OA5tNG466257; Tue, 24 Jan 2023 11:05:55 +0100 Date: Tue, 24 Jan 2023 11:05:55 +0100 From: Jakub Jelinek To: Andrew MacLeod Cc: gcc-patches , "hernandez, aldy" , Richard Biener Subject: Re: [PATCH 2/2] Add VREL_OTHER for FP unsupported relations. Message-ID: Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 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=-3.5 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_H2,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 Mon, Jan 23, 2023 at 12:44:48PM -0500, Andrew MacLeod wrote: > @@ -784,17 +794,28 @@ value_relation::negate () > bool > value_relation::intersect (const value_relation &p) > { > - // Save previous value > - relation_kind old = related; > + relation_kind k; > > if (p.op1 () == op1 () && p.op2 () == op2 ()) > - related = relation_intersect (kind (), p.kind ()); > + k = relation_intersect (kind (), p.kind ()); > else if (p.op2 () == op1 () && p.op1 () == op2 ()) > - related = relation_intersect (kind (), relation_swap (p.kind ())); > + k = relation_intersect (kind (), relation_swap (p.kind ())); > else > return false; > > - return old != related; > + if (related == k) > + return false; > + > + bool float_p = name1 && FLOAT_TYPE_P (TREE_TYPE (name1)); > + if (float_p && p.kind () != k && k == VREL_UNDEFINED) > + { > + if (relation_lt_le_gt_ge_p (kind ()) > + || relation_lt_le_gt_ge_p (p.kind ())) > + k = VREL_OTHER; > + } I don't understand this. When at least one of the relations is VREL_{L,G}{T,E} and intersection is VREL_UNDEFINED, it means other relation is VREL_UNDEFINED (but then VREL_UNDEFINED is the right range to return), or one is VREL_{L,G}T and the other is VREL_EQ (x > y && x == y, again, VREL_UNDEFINED is the right answer, no ordered pair of x and y is greater (or less) and equal at the same time and for unordered (at least one NaN) both relations are false), or it is VREL_LT vs. VREL_G{T,E} or vice versa or VREL_GT vs. VREL_L{T,E} or vice versa (x < y && x >= y again, for ordered pairs of x and y it is never true, if any is NaN, neither comparison is true). I don't think you need to do anything floating point related in intersect. It might seem to be inconsistent with union, but that is just because VREL_OTHER is the union of VREL_LTGT, VREL_ORDERED, VREL_UNORDERED, VREL_UNLT, VREL_UNGT, VREL_UNGT, VREL_UNGE and VREL_UNEQ, if we had those 8 in addition to the current 8 non-PE ones you'd see that for intersections one only gets the new codes when fp with possible NANs and at least one of the intersection operand is one of the new codes. In the VREL_OTHER world, simply VREL_OTHER intersected with anything but VREL_UNDEFINED is VREL_OTHER. > + // (x < y) || (x > y) produces x != y, but this is not true with floats. > + // (x <= y) || (x >= y) produces VARYING, which is also not true for floats. This misses a couple of needed cases both in this comment and in the actual implementation. In particular, the first comment line is right, x < y || x > y is the only one where we get VREL_NE and want VREL_OTHER (VREL_LTGT maybe later). x <= y || x >= y is just one of the cases which produce VREL_VARYING and we want VREL_OTHER (VREL_ORDERED maybe later) though, x < y || x >= y x <= y || x > y are the others where from the current tables you also get VREL_VARYING but want VREL_OTHER (VREL_ORDERED eventually). > + // As they cannot be properly represented, use VREL_OTHER. > + bool float_p = name1 && FLOAT_TYPE_P (TREE_TYPE (name1)); This should be bool float_p = name1 && HONOR_NANS (TREE_TYPE (name1)); If it is not floating point mode, it will be false, if it is floating point mode which doesn't have NANs in hw, it also behaves the same relation-wise, and lastly if hw supports NANs but user uses -ffast-math or -ffinite-math-only, the user guaranteed that the operands will never be NAN (nor +-INF), which means that again relation behave like the integral/pointer ones, there is no 4th possible outcome of a comparison. > + if (float_p && p.kind () != k) > + { > + if (kind () == VREL_LT && p.kind () == VREL_GT) > + k = VREL_OTHER; > + else if (kind () == VREL_GT && p.kind () == VREL_LT) > + k = VREL_OTHER; > + else if (kind () == VREL_LE && p.kind () == VREL_GE) > + k = VREL_OTHER; > + else if (kind () == VREL_GE && p.kind () == VREL_LE) > + k = VREL_OTHER; And as written above, this misses the VREL_LT vs. VREL_GE or VREL_GE vs. VREL_LT or VREL_GT vs. VREL_LE or VREL_LE vs. VREL_GT cases. I think it is easier written as if (k == VREL_VARYING && relation_lt_le_gt_ge_p (kind ()) && relation_lt_le_gt_ge_p (p.kind ())) k = VREL_OTHER; Only when/if we'll want to differentiate between VREL_LTGT for VREL_LT vs. VREL_GT or VREL_GT vs. VREL_LT and VREL_ORDERED for the others we will need something different. > --- a/gcc/value-relation.h > +++ b/gcc/value-relation.h > @@ -70,6 +70,7 @@ typedef enum relation_kind_t > VREL_GE, // r1 >= r2 > VREL_EQ, // r1 == r2 > VREL_NE, // r1 != r2 > + VREL_OTHER, // unrepresentatble floating point relation. s/unrepresentatble/unrepresentable/ > VREL_PE8, // 8 bit partial equivalency > VREL_PE16, // 16 bit partial equivalency > VREL_PE32, // 32 bit partial equivalency Otherwise LGTM (i.e. I agree with VREL_OTHER introduction for now) and the intersect/union tables look good too (I must say I don't understand much the transitive table - how that compares to the intersect one, but that isn't problem in the code, just on my side). Jakub