From patchwork Mon Aug 12 13:33:08 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Law X-Patchwork-Id: 1971586 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=ventanamicro.com header.i=@ventanamicro.com header.a=rsa-sha256 header.s=google header.b=WDfHXjdD; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4WjFpZ5Y1nz1yYC for ; Mon, 12 Aug 2024 23:33:42 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 939AB3858C60 for ; Mon, 12 Aug 2024 13:33:40 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-pl1-x62b.google.com (mail-pl1-x62b.google.com [IPv6:2607:f8b0:4864:20::62b]) by sourceware.org (Postfix) with ESMTPS id 2847A3858D20 for ; Mon, 12 Aug 2024 13:33:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2847A3858D20 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=ventanamicro.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=ventanamicro.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 2847A3858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::62b ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1723469598; cv=none; b=mvHt3vTfFgb1H5WR/cOfIUUJpFzr2s9FmSQ+SszEQZmTMWHpnxC7LFPEINB4Dvu6DKn6ZkOF9xk2HgRe2N/PwVHa5SdiMo42i30UwQh1lC4aI9Tp+ZbxA+miF2d4g92WHZboAF123XJgjJNZiiU41GL3bZM7PnIF3j5VTGSq9bc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1723469598; c=relaxed/simple; bh=2FPppCCF25yAjR5W5UizyLaUxYixsPfUaMAUxe4XXpI=; h=DKIM-Signature:Message-ID:Date:MIME-Version:From:Subject:To; b=tySEwzxWi8YRXjPWscztyNdG7NYX2s9Bc1pchMz4a215nuiWX5/OKfG6Msq7aggWrzNGfOWcDaj0J1l1hHfx/57t+BDywlpy3SsZdtnX/Rhfn8dEXWPqIc55CqxzpNV9aB5fSmTus57wBNdwBuz25TTtKnkQ7DuezHSdDze8pL0= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pl1-x62b.google.com with SMTP id d9443c01a7336-1fd65aaac27so35867275ad.1 for ; Mon, 12 Aug 2024 06:33:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1723469594; x=1724074394; darn=gcc.gnu.org; h=to:content-language:subject:from:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=vF1A3x7oW3koALE1fogxkcUBX4RD6tdIiqvFYTatODQ=; b=WDfHXjdDD6/JohrLKzwanjMn2ZbF058lJBO+UeC82CjqIhM1bqswoq0gJVijGrWSRt Lpdfkp8jmoP2n2b+ky9hhhfI6VHhiGg/vutfAsR04mUMo1v+5dLQqy48Wk7pbh4mByuX pFJFdvL7q80nQKA9PHOnmoe+piuZ/eU4g4jmi+qGo7Wqh2AsINbccq26b6lEHeLOGkAW Lzd/gE1YufzL8XflMvbjfBhx4C9HddYBSkboHHCYfie9i+YJsfJZzCo1Vof7VGBnqu0Q WQE39kApMwzmWI1wdeOE5eyL9in+FOkctOuoEB3glkr+WQVA4j02SoU8AvgUYhFUU33N TVaA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723469594; x=1724074394; h=to:content-language:subject:from:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=vF1A3x7oW3koALE1fogxkcUBX4RD6tdIiqvFYTatODQ=; b=Pw4WnxXOkyn6Qg+bBG4kZqNiyLt5Lw/Rgj6yCXzQQ4HKh8brdPd25ZfZse8yEvV2gb lV11g+Mv+L6wSaUtCZ6QiF0YJZBwSN5/N9t8qwVCtAWnEU7mshrTF/Sh0cWL2MvA2cG9 4OwITEBxSmryp1MuHtdf5DQwRIMbCxm7EP8wU3CuQDz4pKcnt7N4EOvRYmGzJ9gmnnL0 /FV5DKdPGgsx8p5RFa8JJHF4EtHzRNH29ueBD1G7m7YxBkqtImwfXVrrOEaeSCGurfAk 7wi38G/B6Tfmp/OyDcTdAsO2CEJvvT6YFEwSV9u9l+JytMnOl/JGKfKFI0DO1VrBgQBf nS0w== X-Gm-Message-State: AOJu0YzJTTovnbh3elyWfuC/7GD51qihuVhDvmwng63q8u6E0stC+h9M ya7/9uNjWCypnysRUpRKCuE0fHbB9hsEODihlqFkOzUeHyIsnvFGhCiiU8YqGLKFmBHJGCWolZR 9KTY= X-Google-Smtp-Source: AGHT+IEKlBhCpy/IFZs0b2pqPqP+yR4Y2Z/hCBe8t6ePlPP3m+ya0IHsLp6+aJTnZd2blBC7593U5g== X-Received: by 2002:a17:903:2441:b0:1fd:6033:f94e with SMTP id d9443c01a7336-201caa10c4cmr1598155ad.27.1723469593473; Mon, 12 Aug 2024 06:33:13 -0700 (PDT) Received: from [172.31.0.109] ([136.36.72.243]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-200bb7eeeafsm37807005ad.26.2024.08.12.06.33.11 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 12 Aug 2024 06:33:12 -0700 (PDT) Message-ID: Date: Mon, 12 Aug 2024 07:33:08 -0600 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta From: Jeff Law Subject: [committed][rtl-optimization/116244] Don't create bogus regs in alter_subreg Content-Language: en-US To: "gcc-patches@gcc.gnu.org" X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org So this is another nasty latent bug exposed by ext-dce. Similar to the prior m68k failure it's another problem with how we handle paradoxical subregs on big endian targets. In this instance when we remove the hard subregs we take something like: (subreg:DI (reg:SI 0) 0) And turn it into (reg:SI -1) Which is clearly wrong. (reg:SI 0) is correct. The transformation happens in alter_subreg, but I really wanted to fix this in subreg_regno since we could have similar problems in some of the other callers of subreg_regno. Unfortunately reload depends on the current behavior of subreg_regno; in the cases where the return value is an invalid register, the wrong half of a register pair, etc the resulting bogus value is detected by reload and triggers reloading of the inner object. So that's the new comment in subreg_regno. The second best place to fix is alter_subreg which is what this patch does. If presented with a paradoxical subreg, then the base register number should always be REGNO (SUBREG_REG (object)). It's just how paradoxicals are designed to work. I haven't tried to fix the other places that call subreg_regno. After being burned by reload, I'm more than a bit worried about unintended fallout. I must admit I'm surprised we haven't stumbled over this before and that it didn't fix any failures on the big endian embedded targets. Boostrapped & regression tested on x86_64, also went through all the embedded targets in my tester and bootstrapped on m68k & s390x to get some additional big endian testing. Pushing to the trunk. jeff commit e9738e77674e23f600315ca1efed7d1c7944d0cc Author: Jeff Law Date: Mon Aug 12 07:29:25 2024 -0600 [rtl-optimization/116244] Don't create bogus regs in alter_subreg So this is another nasty latent bug exposed by ext-dce. Similar to the prior m68k failure it's another problem with how we handle paradoxical subregs on big endian targets. In this instance when we remove the hard subregs we take something like: (subreg:DI (reg:SI 0) 0) And turn it into (reg:SI -1) Which is clearly wrong. (reg:SI 0) is correct. The transformation happens in alter_subreg, but I really wanted to fix this in subreg_regno since we could have similar problems in some of the other callers of subreg_regno. Unfortunately reload depends on the current behavior of subreg_regno; in the cases where the return value is an invalid register, the wrong half of a register pair, etc the resulting bogus value is detected by reload and triggers reloading of the inner object. So that's the new comment in subreg_regno. The second best place to fix is alter_subreg which is what this patch does. If presented with a paradoxical subreg, then the base register number should always be REGNO (SUBREG_REG (object)). It's just how paradoxicals are designed to work. I haven't tried to fix the other places that call subreg_regno. After being burned by reload, I'm more than a bit worried about unintended fallout. I must admit I'm surprised we haven't stumbled over this before and that it didn't fix any failures on the big endian embedded targets. Boostrapped & regression tested on x86_64, also went through all the embedded targets in my tester and bootstrapped on m68k & s390x to get some additional big endian testing. Pushing to the trunk. rtl-optimization/116244 gcc/ * rtlanal.cc (subreg_regno): Update comment. * final.cc (alter_subrg): Always use REGNO (SUBREG_REG ()) to get the base regsiter for paradoxical subregs. gcc/testsuite/ * g++.target/m68k/m68k.exp: New test driver. * g++.target/m68k/pr116244.C: New test. diff --git a/gcc/final.cc b/gcc/final.cc index eb9e065d9f0..0167b2f8602 100644 --- a/gcc/final.cc +++ b/gcc/final.cc @@ -3123,7 +3123,17 @@ alter_subreg (rtx *xp, bool final_p) unsigned int regno; poly_int64 offset; - regno = subreg_regno (x); + /* A paradoxical should always be REGNO (y) + 0. Using subreg_regno + for something like (subreg:DI (reg:SI N) 0) on a WORDS_BIG_ENDIAN + target will return N-1 which is catastrophic for N == 0 and just + wrong for other cases. + + Fixing subreg_regno would be a better option, except that reload + depends on its current behavior. */ + if (paradoxical_subreg_p (x)) + regno = REGNO (y); + else + regno = subreg_regno (x); if (subreg_lowpart_p (x)) offset = byte_lowpart_offset (GET_MODE (x), GET_MODE (y)); else diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc index 4158a531bdd..6f6e6544755 100644 --- a/gcc/rtlanal.cc +++ b/gcc/rtlanal.cc @@ -4313,7 +4313,16 @@ lowpart_subreg_regno (unsigned int regno, machine_mode xmode, return simplify_subreg_regno (regno, xmode, offset, ymode); } -/* Return the final regno that a subreg expression refers to. */ +/* Return the final regno that a subreg expression refers to. + + Callers such as reload_inner_reg_of_subreg rely on this returning + the simplified hard reg, even if that result is not a valid regno for + the given mode. That triggers reloading the inner part of the + subreg. + + That inherently means other uses of this routine probably need + to be audited for their behavior when requested subreg can't + be expressed as a hard register after apply the offset. */ unsigned int subreg_regno (const_rtx x) { diff --git a/gcc/testsuite/g++.target/m68k/m68k.exp b/gcc/testsuite/g++.target/m68k/m68k.exp new file mode 100644 index 00000000000..8f6416e9fdf --- /dev/null +++ b/gcc/testsuite/g++.target/m68k/m68k.exp @@ -0,0 +1,34 @@ +# Copyright (C) 2019-2024 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with GCC; see the file COPYING3. If not see +# . + +# GCC testsuite that uses the `dg.exp' driver. + +# Exit immediately if this isn't a m68k target. +if ![istarget m68k*-*-*] then { + return +} + +# Load support procs. +load_lib g++-dg.exp + +# Initialize `dg'. +dg-init + +# Main loop. +dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.C]] "" "" + +# All done. +dg-finish diff --git a/gcc/testsuite/g++.target/m68k/pr116244.C b/gcc/testsuite/g++.target/m68k/pr116244.C new file mode 100644 index 00000000000..2e78832efbb --- /dev/null +++ b/gcc/testsuite/g++.target/m68k/pr116244.C @@ -0,0 +1,226 @@ +// { dg-do compile } +// { dg-additional-options "-std=gnu++20 -O2 -w -march=isaa" } +namespace std { +struct __conditional { + template using type = _Tp; +}; +template +using __conditional_t = __conditional::type<_If, _Else>; +template constexpr bool is_lvalue_reference_v = true; +template bool is_same_v; +template +constexpr bool is_convertible_v = __is_convertible(_From, _To); +template bool is_invocable_v; +template using common_reference_t = int; +template struct iterator_traits : _Iterator {}; +namespace __detail { +template +concept __same_as = is_same_v<_Up>; +} +template +concept same_as = __detail::__same_as<_Up, _Tp>; +template +concept derived_from = is_convertible_v<_Derived, _Base>; +template +concept common_reference_with = + same_as, common_reference_t<>>; +namespace __detail { +template +concept __boolean_testable = requires(_Tp __t) { __t; }; +template +concept __weakly_eq_cmp_with = requires(_Tp __t) { __t; }; +} // namespace __detail +template +concept invocable = is_invocable_v<_Args...>; +template +concept regular_invocable = invocable<>; +template +concept predicate = __detail::__boolean_testable<_Fn>; +template +concept relation = predicate<_Rel>; +template +concept strict_weak_order = relation<_Rel, _Tp, _Up>; +namespace { +struct duration { + duration() = default; + template duration(_Rep2 __rep) : __r(__rep) {} + long long count() { return __r; } + long long __r; +}; +long long operator+(duration __lhs, duration __rhs) { + long __trans_tmp_1 = __lhs.count(); + return __trans_tmp_1 + __rhs.count(); +} +struct time_point { + time_point(); + time_point(duration __dur) : __d(__dur) {} + duration time_since_epoch() { return __d; } + duration __d; +}; +time_point operator+(time_point __lhs, duration __rhs) { + duration __trans_tmp_2 = __lhs.time_since_epoch(); + return time_point(__trans_tmp_2 + __rhs); +} +template using sys_time = time_point; +} // namespace +template class allocator; +namespace { +struct less {}; +} // namespace +struct forward_iterator_tag {}; +namespace __detail { +template struct __iter_traits_impl { + using type = iterator_traits<_Iter>; +}; +template using __iter_traits = __iter_traits_impl<_Iter>::type; +template struct __iter_concept_impl; +template + requires requires { typename _Iter; } +struct __iter_concept_impl<_Iter> { + using type = __iter_traits<_Iter>::iterator_concept; +}; +template +using __iter_concept = __iter_concept_impl<_Iter>::type; +template +concept __indirectly_readable_impl = common_reference_with<_In, _In>; +} // namespace __detail +template +concept indirectly_readable = __detail::__indirectly_readable_impl<_In>; +template +concept input_or_output_iterator = requires(_Iter __i) { __i; }; +template +concept sentinel_for = __detail::__weakly_eq_cmp_with<_Sent, _Iter>; +template +concept forward_iterator = + derived_from<__detail::__iter_concept<_Iter>, forward_iterator_tag>; +template +concept indirectly_regular_unary_invocable = regular_invocable<_Fn>; +template +concept indirect_strict_weak_order = strict_weak_order<_Fn, _I1, _I2>; +namespace __detail { +template struct __projected; +} +template _Proj> +using projected = __detail::__projected<_Iter, _Proj>; +namespace ranges::__access { +template auto __begin(_Tp __t) { return __t.begin(); } +} // namespace ranges::__access +namespace __detail { +template +using __range_iter_t = decltype(ranges::__access::__begin(_Tp())); +} +template struct iterator_traits<_Tp *> { + typedef forward_iterator_tag iterator_concept; + using reference = _Tp; +}; +} // namespace std +template struct __normal_iterator { + using iterator_concept = std::__detail::__iter_concept<_Iterator>; + std::iterator_traits<_Iterator>::reference operator*(); + void operator++(); +}; +template +bool operator==(__normal_iterator<_IteratorL, _Container>, + __normal_iterator<_IteratorR, _Container>); +int _M_get_sys_info_ri; +namespace std { +template struct allocator_traits; +template struct allocator_traits> { + using pointer = _Tp *; +}; +namespace { +namespace __detail { +template +concept __maybe_borrowed_range = is_lvalue_reference_v<_Tp>; +} +int end; +template +concept range = requires { end; }; +template +concept borrowed_range = __detail::__maybe_borrowed_range<_Tp>; +template using iterator_t = std::__detail::__range_iter_t<_Tp>; +template +concept forward_range = forward_iterator>; +struct dangling; +template _Sent = _It> +struct subrange { + _It begin(); + _Sent end(); +}; +template +using borrowed_subrange_t = + __conditional_t, subrange>, + dangling>; +} // namespace +template struct _Vector_base { + typedef allocator_traits<_Alloc>::pointer pointer; +}; +template > struct vector { + __normal_iterator::pointer, int> begin(); +}; +template struct __shared_ptr_access { + _Tp *operator->(); +}; +namespace chrono { +struct weekday { + char _M_wd; + weekday _S_from_days() { + long __trans_tmp_3; + return 7 > __trans_tmp_3; + } + weekday(unsigned __wd) : _M_wd(__wd == 7 ?: __wd) {} + weekday() : weekday{_S_from_days()} {} + unsigned c_encoding() { return _M_wd; } + friend duration operator-(weekday __x, weekday __y) { + auto __n = __x.c_encoding() - __y.c_encoding(); + return int(__n) >= 0 ? __n : duration{}; + } +}; +struct year_month_day { + year_month_day _S_from_days(duration __dp) { + auto __r0 = int(__dp.count()), __u2 = 5 * __r0; + year_month_day{__u2}; + } + year_month_day(); + year_month_day(int); + year_month_day(sys_time __dp) + : year_month_day(_S_from_days(__dp.time_since_epoch())) {} +}; +struct time_zone { + void _M_get_sys_info() const; +}; +} // namespace chrono +namespace ranges { +struct { + template < + forward_range _Range, typename _Tp, typename _Proj, + indirect_strict_weak_order<_Tp, projected<_Range, _Proj>> _Comp = less> + borrowed_subrange_t<_Range> operator()(_Range, _Tp, _Comp, _Proj); +} equal_range; +} // namespace ranges +} // namespace std +namespace std::chrono { +struct Rule; +unsigned day_of_week; +struct _Node { + vector rules; +}; +char name; +struct Rule { + int from; + void start_time(int, long) { + year_month_day ymd; + sys_time date; + duration diff = day_of_week - weekday{}; + ymd = date + diff; + } +}; +__shared_ptr_access<_Node> __trans_tmp_5; +void time_zone::_M_get_sys_info() const { + auto node = __trans_tmp_5; + auto rules = ranges::equal_range(node->rules, _M_get_sys_info_ri, {}, name); + for (auto rule : rules) + rule.start_time(rule.from, {}); +} +} // namespace std::chrono