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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 6002B3839821 for ; Wed, 16 Jun 2021 16:05:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6002B3839821 Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-205-6iNTbvNzPXytld5x4vbVLA-1; Wed, 16 Jun 2021 12:05:41 -0400 X-MC-Unique: 6iNTbvNzPXytld5x4vbVLA-1 Received: by mail-qk1-f199.google.com with SMTP id 14-20020a37060e0000b02903aad32851d2so2154414qkg.1 for ; Wed, 16 Jun 2021 09:05:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=YyhZJjB2BJtudWcvF39yJ4nMNJNRscrIysLnu3RJKss=; b=tHv2FLJJGLpYHbCEg/naEU5bQ5yP4jsXM82b0bJR0OVQTDKWIEv+BewOFYr5r7bmyF cfQIALrq4zynp9Bx7gyYG0baHB/vWZYLniHig/+PRvu8M01sBEiYzQtQR42H31ZN8rKE AEVi3dh+CxuZoBqZM5E+zPS00wv8gIyFzTUZrZYYOV/Untd/D2rzajOpIeVZxUPFGDFr 4vLeg2OFC9RK7UtUKQK0X/5kjEDUL5ij/c9AuqWPVSEFEUlowGczVjpf7NAnBWog54xZ uYKlqnA4Qv1E4zn9Qti6acIOQIUE+bEoLRaW4FPaKVNG53695R5pSHjbG7gnum0WXaEQ /NUA== X-Gm-Message-State: AOAM532T/0Hs1ZbzqAF/7atdoSIdAP5dBpbndGBfI33kxr/eGkL55Cbe mVaubjLZXKigkvVhgI2FFPWrVXBmfoc4eiLQIGhkUb0N17hjTzZnZB/N5KzQW3kAFt+K6XaHQil NyrgldNPVKi0R/7cCHAAIJ1FTzGlFFVsRwfmyYa+boSbjtvk7EXeb9hZR2ctc98uRGLu9lA== X-Received: by 2002:ac8:7207:: with SMTP id a7mr611765qtp.32.1623859540459; Wed, 16 Jun 2021 09:05:40 -0700 (PDT) X-Google-Smtp-Source: ABdhPJx6bOKJjWfVENi/StskmmD/PiYtay1HJB1EHenOXjp2B6iznLljfywqL2fwMwVfrPD5ySa3TQ== X-Received: by 2002:ac8:7207:: with SMTP id a7mr611739qtp.32.1623859540187; Wed, 16 Jun 2021 09:05:40 -0700 (PDT) Received: from ?IPv6:2607:fea8:a241:4600::58f9? ([2607:fea8:a241:4600::58f9]) by smtp.gmail.com with ESMTPSA id y25sm1970097qkj.48.2021.06.16.09.05.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 16 Jun 2021 09:05:39 -0700 (PDT) Subject: Re: [PATCH] tree-optimization PR/101014 - Limit new value calculations to first order effects. To: Maxim Kuvyrkov Cc: gcc-patches References: <51d7f2dc-e7ff-d136-aacc-7d3a4fb28dc4@redhat.com> <0D8A2269-0C8E-48A7-B117-AAC107EE0C4B@linaro.org> From: Andrew MacLeod Message-ID: Date: Wed, 16 Jun 2021 12:05:38 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <0D8A2269-0C8E-48A7-B117-AAC107EE0C4B@linaro.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-CA X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Jun 2021 16:05:44 -0000 On 6/16/21 5:41 AM, Maxim Kuvyrkov wrote: >> On 15 Jun 2021, at 00:07, Andrew MacLeod via Gcc-patches wrote: >> >> As mentioned in the Text from the PR: >> >> "When a range is being calculated for an ssa-name, the propagation process often goes along back edges. These back edges sometime require other ssa-names which have not be processed yet. These are flagged as "poor values" and when propagation is done, we visit the list of poor values, calculate them, and see if that may result if a better range for the original ssa-name. >> >> The problem is that calculating these poor values may also spawn another set of requests since the block at the far end of the back edge has not been processed yet... its highly likely that some additional unprocessed ssa-names are used in the calculation of that name, but typically they do not affect the current range in a significant way. >> >> Thus we mostly we care about the first order effect only. It turns out to be very rare that a 2nd order effect on a back edge affects anything that we don't catch later. >> >> This patch turns off poor-value tagging when looking up the first order values, thus avoiding the 2nd order and beyond cascading effects. >> >> I haven't found a test case we miss yet because of this change, yet it probably resolves a number of the outstanding compilation problems in a significant way. >> >> I think this will probably apply to gcc 11 in some form as well, so I'll look at an equivalent patch for there." >> >> >> This patch simplifies the enable_new_value routines.. replacing the enable/disable with an enable with flag routine, which returns the previous value.This lets us change the mode and then set it back to what it was before. Seems better in general. >> >> Then disables new values for 2nd+ order effects. GCC11 patch forthcoming. >> >> Bootstraps on x86_64-pc-linux-gnu, no regressions. pushed. >> >> Andrew > Hi Andrew, > > This causes bootstrap-with-ubsan failure on at least aarch64-linux-gnu, likely, others: > > # 00:42:32 /home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8: runtime error: load of value 48, which is not a valid value for type 'bool' > # 00:42:32 /home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8: runtime error: load of value 48, which is not a valid value for type 'bool' > # 00:42:32 /home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8: runtime error: load of value 32, which is not a valid value for type 'bool' > # 00:42:32 /home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8: runtime error: load of value 48, which is not a valid value for type 'bool' > # 00:42:32 /home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8: runtime error: load of value 32, which is not a valid value for type 'bool' > # 00:42:32 /home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8: runtime error: load of value 48, which is not a valid value for type 'bool' > # 00:42:32 /home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8: runtime error: load of value 32, which is not a valid value for type 'bool' > # 00:42:32 /home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8: runtime error: load of value 32, which is not a valid value for type 'bool' > >> @@ -748,21 +748,15 @@ ranger_cache::dump (FILE *f) >> fprintf (f, "\n"); >> } >> >> -// Allow the cache to flag and query new values when propagation is forced >> -// to use an unknown value. >> +// Allow or disallow the cache to flag and query new values when propagation >> +// is forced to use an unknown value. The previous state is returned. >> >> -void >> -ranger_cache::enable_new_values () >> -{ >> - m_new_value_p = true; >> -} >> - >> -// Disable new value querying. >> - >> -void >> -ranger_cache::disable_new_values () >> +bool >> +ranger_cache::enable_new_values (bool state) >> { >> - m_new_value_p = false; >> + bool ret = m_new_value_p; > I think changing this to > bool ret = (bool) m_new_value_p; > might be enough, but you know this code better. > > Would you please take a look at this? > >> + m_new_value_p = state; >> + return ret; >> } >> >> // Dump the caches for basic block BB to file F. > Thanks, > Huh, not sure why that would matter since m_new_value_p is a bool. My guess is (and this bugged me after I checked it in, just haven't gotten to it yet), is that this is initialized in the constructor with a call, and the return value ignored.  Which means there is techincally a load of an uninitialized value, which is then ignored.  but the load may happen.  Im going to guess thats the issue.  It needs fixing anyway Im testing this fix, which i will check in.  See if that solves the ubsan issue. @@ -727,7 +727,7 @@ ranger_cache::ranger_cache (gimple_ranger &q) : query (q)        if (bb)         m_gori.exports (bb);      } -  enable_new_values (true); +  m_new_value_p = true;  }