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.133.124]) by sourceware.org (Postfix) with ESMTPS id C3F803858D33 for ; Tue, 6 Jun 2023 12:43:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C3F803858D33 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=1686055418; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ecPMfxbtSusDb/DBDusmymIlOIXvaPMrG/AcG9CgEvM=; b=Mn6JYsOEcVxu79xJ1B7nxazRMuXVq8DdaARFMk8IkCm03TTsJAoEsA3xT5gckaF+SwpxLr mFzuQD83bRvVtl3aRTVEFp647SpkShejM6QLQKJ8q7r3mx9OiaTsWEJMf/SdQgQaSx2z7y 9xLrTaOQEU2WjSJeFW0oukwW1MDbuuY= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-336-HkHDea5HNDi6SMHDjw2eHQ-1; Tue, 06 Jun 2023 08:43:37 -0400 X-MC-Unique: HkHDea5HNDi6SMHDjw2eHQ-1 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-30ae18b11bfso2637580f8f.0 for ; Tue, 06 Jun 2023 05:43:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686055416; x=1688647416; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ecPMfxbtSusDb/DBDusmymIlOIXvaPMrG/AcG9CgEvM=; b=ETEDw4DV2zunqEFPR0aKxiw1cqg0hiTa5/MIuBcbOmYFAQcfcRWCbg+CIWWX6irtZ0 dt6w1UVjhKrvG6FO0tof2cIlSa5tDhT/ZawyUZQoSq2/xp7bJUuWM80gD+hjiKgjwe46 6AUwMn8qnquZ1r1sW7S1zUtx+p0gFUq8zz2+rDPh/Y0BMTTsK+vFycT/b6OeTAnl+PT7 vjW2Rg4Pc52z9dnA5DSbCrM5OLQJHVRvD2TuSG7te/kOI7cvHYmgWU4n7DGLYLzu8aY3 Tm8SmryGz+WA16P67e8HUkC9XF6TMmtQn2UqyVoZp90KxHB5MLQRoEtxWo7dgsjhzLLB 2QBg== X-Gm-Message-State: AC+VfDw3lmacybg8Blru4JHd1Nga0m5IByqX1xaJxLc6rX5iPbkxXK41 IeFOmUnEc5Q0cvyc4gchiMUMLiqQ6hB0VtDYUweLNYj6Px8ImJPGjwCKnHedCZs4AsbVMAxKjbD KE7Oi8bzs7xSNAMkuVQ== X-Received: by 2002:adf:e948:0:b0:30a:e3bb:ba8b with SMTP id m8-20020adfe948000000b0030ae3bbba8bmr8284903wrn.29.1686055416172; Tue, 06 Jun 2023 05:43:36 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7F8qaHavt2uVySqlwK94YJLtU0NRK7R3246U+EjZZMbwqXJt2bjOKAlyV3UxLOhkg6O6V5Bw== X-Received: by 2002:adf:e948:0:b0:30a:e3bb:ba8b with SMTP id m8-20020adfe948000000b0030ae3bbba8bmr8284893wrn.29.1686055415763; Tue, 06 Jun 2023 05:43:35 -0700 (PDT) Received: from [192.168.1.201] ([139.47.42.170]) by smtp.gmail.com with ESMTPSA id p25-20020a7bcc99000000b003f4289b18a7sm13960309wma.5.2023.06.06.05.43.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 06 Jun 2023 05:43:35 -0700 (PDT) Message-ID: <95c314f9-4836-d693-5d04-879b2b6face2@redhat.com> Date: Tue, 6 Jun 2023 14:43:34 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Subject: Re: [PATCH] Convert ipcp_vr_lattice to type agnostic framework. To: Martin Jambor Cc: Andrew MacLeod , GCC patches References: From: Aldy Hernandez In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: My apologies for the delay. I was on vacation. On 5/26/23 18:17, Martin Jambor wrote: > Hello, > > On Mon, May 22 2023, Aldy Hernandez wrote: >> I've adjusted the patch with some minor cleanups that came up when I >> implemented the rest of the IPA revamp. >> >> Rested. OK? >> >> On Wed, May 17, 2023 at 4:31 PM Aldy Hernandez wrote: >>> >>> This converts the lattice to store ranges in Value_Range instead of >>> value_range (*) to make it type agnostic, and adjust all users >>> accordingly. >>> >>> I think it is a good example on converting from static ranges to more >>> general, type agnostic ones. >>> >>> I've been careful to make sure Value_Range never ends up on GC, since >>> it contains an int_range_max and can expand on-demand onto the heap. >>> Longer term storage for ranges should be done with vrange_storage, as >>> per the previous patch ("Provide an API for ipa_vr"). >>> >>> (*) I do know the Value_Range naming versus value_range is quite >>> annoying, but it was a judgement call last release for the eventual >>> migration to having "value_range" be a type agnostic range object. We >>> will ultimately rename Value_Range to value_range. > > It is quite confusing for an unsuspecting reader indeed. > >>> >>> OK for trunk? > > I guess I need to rely on that you know what you are doing :-) > I have seen in other messages that you measure the compile time > effects of your patches, do you look at memory use as well? Before going in depth into the rest of your review (thanks BTW), let's address memory usage. To be honest, I didn't measure memory, but only because I had a pretty good inkling that it wouldn't make a difference, since vrange_storage is designed to take less space than value_range which you were using. But you're right, I should've measured it. First value_range is a derived-POD so it has a vtable pointer in there. vrange_storage does not. Second, vrange_storage only uses the minimum number of bytes to represent the integers in the range. vrange_storage uses a trailing_wide_int type mechanism to store the minimum amount of HWIs for a range. See irange_storage::set_irange(). value_range is a typedef for int_range<2>. Storing this in GC, which IPA currently does, takes 432 bytes. I will be removing the GTY markers for irange, to keep anyone from even trying. On the other hand, storing this same range in GC memory with vrange_storage takes 35 bytes for a [5, 10] range: unsigned prec = TYPE_PRECISION (integer_type_node); wide_int min = wi::shwi (5, prec); wide_int max = wi::shwi (10, prec); int_range<2> r (integer_type_node, min, max); vrange_storage *p = ggc_alloc_vrange_storage (r); Breaking on irange_storage::alloc() you can see the size of the allocated object is 35 bytes. This is far less than 432 bytes in trunk (due to the wide_int penalty in this release), but even so is comparable to GCC12 which took 32 bytes. Note that GCC12 was using trees, so those 32 bytes were deceptive, since there was another level of indirection involved for the actual HWIs. Anywhoo... I tried comparing GCC 12 to current mainline plus these patches, but it was a royal PITA, because so much has changed, not just in the ranger world, but in the rest of the compiler. However, comparing trunk against my patches is a total wash, even considering that IPA will now store a gazillion more ranges. For measuring I built with --enable-gather-detailed-mem-stats and compared the "Total Allocated:" lines from -fmem-report for the aggregate of all .ii files in a bootstrap: Before: Total allocated: 73360474112.0 bytes After: Total allocated: 73354182656.0 bytes So we use 0.00858% less memory. To be honest, this is an unfair comparison because trunk IPA is streaming out the full value_range (wide_int's and all which changed in this release), but with these patches: a) We don't stream out vtable pointer. b) Even if the number of sub-ranges can be larger, we only store the bare minimum, and we're capped at 255 sub-ranges (which I've never seen in the wild). I think we're good, but if this ever becomes a problem, the constructor for ipa_vr could just be tweaked to squish things down before allocating: ipa_vr::ipa_vr (const vrange &v) : m_type (v.type ()) { if (is_a (v)) { int_range<10> squish (as_a (v)); m_storage = ggc_alloc_vrange_storage (squish); } else m_storage = ggc_alloc_vrange_storage (v); } I'll address the rest of your comments in follow-up mails. Aldy