| Message ID | aa1a8148-57ed-47e2-ba3a-9f707fd4f428@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed,rtl-optimization/116244] Don't create bogus regs in alter_subreg | expand |
Jeff Law <jlaw@ventanamicro.com> writes: > 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 <jlaw@ventanamicro.com> > 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); Are you sure that's right? For a 32-bit big-endian target, (subreg:DI (reg:SI 1) 0) really should simplify to (reg:DI 0) rather than (reg:DI 1). Like you say, this leads to an invalid result for (reg:SI 0) in place of (reg:SI 1). But AIUI, it's reload's/LRA's job to ensure that that never happens. This is part of the reason why LRA/IRA need to track the widest referenced mode. So it sounds like something might have gone wrong earlier/elsewhere. Thanks, Richard > 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 > +# <http://www.gnu.org/licenses/>. > + > +# 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 <typename _Tp, typename> using type = _Tp; > +}; > +template <bool, typename _If, typename _Else> > +using __conditional_t = __conditional::type<_If, _Else>; > +template <typename> constexpr bool is_lvalue_reference_v = true; > +template <typename> bool is_same_v; > +template <typename _From, typename _To> > +constexpr bool is_convertible_v = __is_convertible(_From, _To); > +template <typename> bool is_invocable_v; > +template <typename...> using common_reference_t = int; > +template <typename _Iterator> struct iterator_traits : _Iterator {}; > +namespace __detail { > +template <typename, typename _Up> > +concept __same_as = is_same_v<_Up>; > +} > +template <typename _Tp, typename _Up> > +concept same_as = __detail::__same_as<_Up, _Tp>; > +template <typename _Derived, typename _Base> > +concept derived_from = is_convertible_v<_Derived, _Base>; > +template <typename, typename> > +concept common_reference_with = > + same_as<common_reference_t<>, common_reference_t<>>; > +namespace __detail { > +template <typename _Tp> > +concept __boolean_testable = requires(_Tp __t) { __t; }; > +template <typename _Tp, typename> > +concept __weakly_eq_cmp_with = requires(_Tp __t) { __t; }; > +} // namespace __detail > +template <typename... _Args> > +concept invocable = is_invocable_v<_Args...>; > +template <typename> > +concept regular_invocable = invocable<>; > +template <typename _Fn> > +concept predicate = __detail::__boolean_testable<_Fn>; > +template <typename _Rel, typename, typename> > +concept relation = predicate<_Rel>; > +template <typename _Rel, typename _Tp, typename _Up> > +concept strict_weak_order = relation<_Rel, _Tp, _Up>; > +namespace { > +struct duration { > + duration() = default; > + template <typename _Rep2> 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 <typename> using sys_time = time_point; > +} // namespace > +template <typename> class allocator; > +namespace { > +struct less {}; > +} // namespace > +struct forward_iterator_tag {}; > +namespace __detail { > +template <typename _Iter> struct __iter_traits_impl { > + using type = iterator_traits<_Iter>; > +}; > +template <typename _Iter> using __iter_traits = __iter_traits_impl<_Iter>::type; > +template <typename> struct __iter_concept_impl; > +template <typename _Iter> > + requires requires { typename _Iter; } > +struct __iter_concept_impl<_Iter> { > + using type = __iter_traits<_Iter>::iterator_concept; > +}; > +template <typename _Iter> > +using __iter_concept = __iter_concept_impl<_Iter>::type; > +template <typename _In> > +concept __indirectly_readable_impl = common_reference_with<_In, _In>; > +} // namespace __detail > +template <typename _In> > +concept indirectly_readable = __detail::__indirectly_readable_impl<_In>; > +template <typename _Iter> > +concept input_or_output_iterator = requires(_Iter __i) { __i; }; > +template <typename _Sent, typename _Iter> > +concept sentinel_for = __detail::__weakly_eq_cmp_with<_Sent, _Iter>; > +template <typename _Iter> > +concept forward_iterator = > + derived_from<__detail::__iter_concept<_Iter>, forward_iterator_tag>; > +template <typename _Fn, typename> > +concept indirectly_regular_unary_invocable = regular_invocable<_Fn>; > +template <typename _Fn, typename _I1, typename _I2> > +concept indirect_strict_weak_order = strict_weak_order<_Fn, _I1, _I2>; > +namespace __detail { > +template <typename, typename> struct __projected; > +} > +template <indirectly_readable _Iter, > + indirectly_regular_unary_invocable<_Iter> _Proj> > +using projected = __detail::__projected<_Iter, _Proj>; > +namespace ranges::__access { > +template <typename _Tp> auto __begin(_Tp __t) { return __t.begin(); } > +} // namespace ranges::__access > +namespace __detail { > +template <typename _Tp> > +using __range_iter_t = decltype(ranges::__access::__begin(_Tp())); > +} > +template <typename _Tp> struct iterator_traits<_Tp *> { > + typedef forward_iterator_tag iterator_concept; > + using reference = _Tp; > +}; > +} // namespace std > +template <typename _Iterator, typename> struct __normal_iterator { > + using iterator_concept = std::__detail::__iter_concept<_Iterator>; > + std::iterator_traits<_Iterator>::reference operator*(); > + void operator++(); > +}; > +template <typename _IteratorL, typename _IteratorR, typename _Container> > +bool operator==(__normal_iterator<_IteratorL, _Container>, > + __normal_iterator<_IteratorR, _Container>); > +int _M_get_sys_info_ri; > +namespace std { > +template <typename> struct allocator_traits; > +template <typename _Tp> struct allocator_traits<allocator<_Tp>> { > + using pointer = _Tp *; > +}; > +namespace { > +namespace __detail { > +template <typename _Tp> > +concept __maybe_borrowed_range = is_lvalue_reference_v<_Tp>; > +} > +int end; > +template <typename> > +concept range = requires { end; }; > +template <typename _Tp> > +concept borrowed_range = __detail::__maybe_borrowed_range<_Tp>; > +template <typename _Tp> using iterator_t = std::__detail::__range_iter_t<_Tp>; > +template <typename _Tp> > +concept forward_range = forward_iterator<iterator_t<_Tp>>; > +struct dangling; > +template <input_or_output_iterator _It, sentinel_for<_It> _Sent = _It> > +struct subrange { > + _It begin(); > + _Sent end(); > +}; > +template <range _Range> > +using borrowed_subrange_t = > + __conditional_t<borrowed_range<_Range>, subrange<iterator_t<_Range>>, > + dangling>; > +} // namespace > +template <typename _Alloc> struct _Vector_base { > + typedef allocator_traits<_Alloc>::pointer pointer; > +}; > +template <typename _Tp, typename _Alloc = allocator<_Tp>> struct vector { > + __normal_iterator<typename _Vector_base<_Alloc>::pointer, int> begin(); > +}; > +template <typename _Tp> 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<long> __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<Rule> rules; > +}; > +char name; > +struct Rule { > + int from; > + void start_time(int, long) { > + year_month_day ymd; > + sys_time<long> 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
On 8/12/24 1:49 PM, Richard Sandiford wrote: >> >> - 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); > > Are you sure that's right? For a 32-bit big-endian target, > (subreg:DI (reg:SI 1) 0) really should simplify to (reg:DI 0) rather > than (reg:DI 1). Correct, we want to get (reg:DI 0). We get "0" back from REGNO (y). And we get 0 back from byte_lowpart_offset (remember, it's paradoxical). The sum is 0 resulting in (reg:DI 0). > Like you say, this leads to an invalid result for (reg:SI 0) in place > of (reg:SI 1). But AIUI, it's reload's/LRA's job to ensure that that > never happens. This is part of the reason why LRA/IRA need to track > the widest referenced mode. > > So it sounds like something might have gone wrong earlier/elsewhere. Not that I could find, but maybe I missed something. It's been a long time since I wandered through reload. jeff
Jeff Law <jeffreyalaw@gmail.com> writes: > On 8/12/24 1:49 PM, Richard Sandiford wrote: > >>> >>> - 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); >> >> Are you sure that's right? For a 32-bit big-endian target, >> (subreg:DI (reg:SI 1) 0) really should simplify to (reg:DI 0) rather >> than (reg:DI 1). > Correct, we want to get (reg:DI 0). We get "0" back from REGNO (y). > And we get 0 back from byte_lowpart_offset (remember, it's paradoxical). > The sum is 0 resulting in (reg:DI 0). But in my example, REGNO (y) is 1 (it's a different example from the one that prompted the patch). The simplified subreg should be the (reg:DI 0) given by subreg_regno, rather than the (reg:DI 1) given by using REGNO (y). E.g.: (set (reg:SI 1) (mem:SI ADDR)) (set (reg:DI 2) (and:DI (subreg:DI (reg:SI 1) 0) (const_int 127))) should (on big-endian targets) be equivalent to: (set (reg:SI 1) (mem:SI ADDR)) (set (reg:DI 2) (and:DI (reg:DI 0) (const_int 127))) so that r2 is set to 0 and r3 is set to (mem:SI ADDR) & 127. If instead we simplified it to: (set (reg:SI 1) (mem:SI ADDR)) (set (reg:DI 2) (and:DI (reg:DI 1) (const_int 127))) then r2 would be set to 0 and r3 would be set to an indeterminate value & 127. Thanks, Richard > > > > Like you say, this leads to an invalid result for (reg:SI 0) in place >> of (reg:SI 1). But AIUI, it's reload's/LRA's job to ensure that that >> never happens. This is part of the reason why LRA/IRA need to track >> the widest referenced mode. >> >> So it sounds like something might have gone wrong earlier/elsewhere. > Not that I could find, but maybe I missed something. It's been a long > time since I wandered through reload. > > jeff
On 8/12/24 4:02 PM, Richard Sandiford wrote: > Jeff Law <jeffreyalaw@gmail.com> writes: >> On 8/12/24 1:49 PM, Richard Sandiford wrote: >> >>>> >>>> - 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); >>> >>> Are you sure that's right? For a 32-bit big-endian target, >>> (subreg:DI (reg:SI 1) 0) really should simplify to (reg:DI 0) rather >>> than (reg:DI 1). >> Correct, we want to get (reg:DI 0). We get "0" back from REGNO (y). >> And we get 0 back from byte_lowpart_offset (remember, it's paradoxical). >> The sum is 0 resulting in (reg:DI 0). > > But in my example, REGNO (y) is 1 (it's a different example from the > one that prompted the patch). The simplified subreg should be the > (reg:DI 0) given by subreg_regno, rather than the (reg:DI 1) given > by using REGNO (y). > > E.g.: > > (set (reg:SI 1) (mem:SI ADDR)) > (set (reg:DI 2) (and:DI (subreg:DI (reg:SI 1) 0) > (const_int 127))) > > should (on big-endian targets) be equivalent to: > > (set (reg:SI 1) (mem:SI ADDR)) > (set (reg:DI 2) (and:DI (reg:DI 0) > (const_int 127))) > > so that r2 is set to 0 and r3 is set to (mem:SI ADDR) & 127. > > If instead we simplified it to: > > (set (reg:SI 1) (mem:SI ADDR)) > (set (reg:DI 2) (and:DI (reg:DI 1) > (const_int 127))) > > then r2 would be set to 0 and r3 would be set to an indeterminate > value & 127. I've gone back and forth over this before I sent that patch. I can certainly see your logic. I'd convinced myself that the subreg should have simplified to (reg:DI 1). And the inconsistency was driving me bananas as my mental model is that (reg:DI N) covers N and N+1 and all that changes in the order based on endianness. ie, if we have (set (reg:DI 0) (...)) that changes d0/d1. But maybe that's just 20 years of little endian thinking creeping in. In which case (subreg:DI (reg:SI d0) 0) is actually meaningless. Jeff
Jeff Law <jlaw@ventanamicro.com> writes: > On 8/12/24 4:02 PM, Richard Sandiford wrote: >> Jeff Law <jeffreyalaw@gmail.com> writes: >>> On 8/12/24 1:49 PM, Richard Sandiford wrote: >>> >>>>> >>>>> - 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); >>>> >>>> Are you sure that's right? For a 32-bit big-endian target, >>>> (subreg:DI (reg:SI 1) 0) really should simplify to (reg:DI 0) rather >>>> than (reg:DI 1). >>> Correct, we want to get (reg:DI 0). We get "0" back from REGNO (y). >>> And we get 0 back from byte_lowpart_offset (remember, it's paradoxical). >>> The sum is 0 resulting in (reg:DI 0). >> >> But in my example, REGNO (y) is 1 (it's a different example from the >> one that prompted the patch). The simplified subreg should be the >> (reg:DI 0) given by subreg_regno, rather than the (reg:DI 1) given >> by using REGNO (y). >> >> E.g.: >> >> (set (reg:SI 1) (mem:SI ADDR)) >> (set (reg:DI 2) (and:DI (subreg:DI (reg:SI 1) 0) >> (const_int 127))) >> >> should (on big-endian targets) be equivalent to: >> >> (set (reg:SI 1) (mem:SI ADDR)) >> (set (reg:DI 2) (and:DI (reg:DI 0) >> (const_int 127))) >> >> so that r2 is set to 0 and r3 is set to (mem:SI ADDR) & 127. >> >> If instead we simplified it to: >> >> (set (reg:SI 1) (mem:SI ADDR)) >> (set (reg:DI 2) (and:DI (reg:DI 1) >> (const_int 127))) >> >> then r2 would be set to 0 and r3 would be set to an indeterminate >> value & 127. > I've gone back and forth over this before I sent that patch. I can > certainly see your logic. I'd convinced myself that the subreg should > have simplified to (reg:DI 1). > > And the inconsistency was driving me bananas as my mental model is that > (reg:DI N) covers N and N+1 and all that changes in the order based on > endianness. ie, if we have (set (reg:DI 0) (...)) that changes d0/d1. > But maybe that's just 20 years of little endian thinking creeping in. > > In which case (subreg:DI (reg:SI d0) 0) is actually meaningless. Yeah. The same problem can happen for little-endian too, with e.g. a doubleword paradoxical subreg of the final hard register leaking into the virtual registers. But the failure mode tends to be less brutal then. That case is probably more like big-endian (subreg:DI (reg:SI a0) 0), where the combination d7+a0 would be rejected for register class reasons. Richard
On 8/13/24 3:19 AM, Richard Sandiford wrote: >> >> And the inconsistency was driving me bananas as my mental model is that >> (reg:DI N) covers N and N+1 and all that changes in the order based on >> endianness. ie, if we have (set (reg:DI 0) (...)) that changes d0/d1. >> But maybe that's just 20 years of little endian thinking creeping in. >> >> In which case (subreg:DI (reg:SI d0) 0) is actually meaningless. > > Yeah. The same problem can happen for little-endian too, with e.g. > a doubleword paradoxical subreg of the final hard register leaking > into the virtual registers. But the failure mode tends to be less > brutal then. That case is probably more like big-endian > (subreg:DI (reg:SI a0) 0), where the combination d7+a0 would be > rejected for register class reasons. I think the only sensible move is to revert, re-open the bz and re-analyze. Damn. Back into reload ;( jeff
On 8/12/24 3:50 PM, Jeff Law wrote: > > > On 8/12/24 1:49 PM, Richard Sandiford wrote: > >>> - 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); >> >> Are you sure that's right? For a 32-bit big-endian target, >> (subreg:DI (reg:SI 1) 0) really should simplify to (reg:DI 0) rather >> than (reg:DI 1). > Correct, we want to get (reg:DI 0). We get "0" back from REGNO (y). And > we get 0 back from byte_lowpart_offset (remember, it's paradoxical). > The sum is 0 resulting in (reg:DI 0). So rewinding this discussion a bit. Focusing on this insn: > (insn 77 75 80 6 (parallel [ > (set (reg:DI 75 [ _32 ]) > (plus:DI (reg:DI 73 [ _31 ]) > (subreg:DI (reg/v:SI 41 [ __n ]) 0))) > (clobber (scratch:SI)) > ]) "j.C":50:38 discrim 1 155 {adddi3} > (expr_list:REG_DEAD (reg:DI 73 [ _31 ]) > (expr_list:REG_DEAD (reg/v:SI 41 [ __n ]) > (nil)))) Not surprisingly we're focused on the subreg expression in there. The first checkpoint in my mind is IRA's allocation where we assign it to reg 0. > Popping a0(r41,l0) -- assign reg 0 So given the use inside a paradoxical subreg, do we consider this valid? After the discussion from last week, I'm leaning a bit more towards no than before. Let's take a simpler case, the meaning of: (subreg:DI (reg:SI 1) 0) Actually refers to d0, not d1 on the m68k. If we agree on that, then (subreg:DI (reg:SI 0) 0) Logically makes no sense since we can't reference register -1. Note that Georg has a roughly similar looking issue on the avr, but at the high end of the register file (little endian target) which would roughly correspond to the discussion we had last week about paradoxicals on little endian targets. In both cases I'm thinking now that the problem is really a failure to properly define HARD_REGNO_MODE_OK, particularly around boundary conditions where the multi-reg mode either can't be represented or crosses into other physical registers that have fixed uses. Jeff
On 8/18/24 10:40 AM, Jeff Law wrote: > > After the discussion from last week, I'm leaning a bit more towards no > than before. > > Let's take a simpler case, the meaning of: > > (subreg:DI (reg:SI 1) 0) > > Actually refers to d0, not d1 on the m68k. If we agree on that, then > > (subreg:DI (reg:SI 0) 0) > > Logically makes no sense since we can't reference register -1. > > Note that Georg has a roughly similar looking issue on the avr, but at > the high end of the register file (little endian target) which would > roughly correspond to the discussion we had last week about paradoxicals > on little endian targets. > > > In both cases I'm thinking now that the problem is really a failure to > properly define HARD_REGNO_MODE_OK, particularly around boundary > conditions where the multi-reg mode either can't be represented or > crosses into other physical registers that have fixed uses. The alternative is to consider that subreg expression after IRA and that reload/LRA is expected to clean it up. It's a legitimate argument and in some ways reload is already expecting to do this cleanup. jeff
Jeff Law <jeffreyalaw@gmail.com> writes: > On 8/12/24 3:50 PM, Jeff Law wrote: >> >> >> On 8/12/24 1:49 PM, Richard Sandiford wrote: >> >>>> - 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); >>> >>> Are you sure that's right? For a 32-bit big-endian target, >>> (subreg:DI (reg:SI 1) 0) really should simplify to (reg:DI 0) rather >>> than (reg:DI 1). >> Correct, we want to get (reg:DI 0). We get "0" back from REGNO (y). And >> we get 0 back from byte_lowpart_offset (remember, it's paradoxical). >> The sum is 0 resulting in (reg:DI 0). > So rewinding this discussion a bit. > > Focusing on this insn: > >> (insn 77 75 80 6 (parallel [ >> (set (reg:DI 75 [ _32 ]) >> (plus:DI (reg:DI 73 [ _31 ]) >> (subreg:DI (reg/v:SI 41 [ __n ]) 0))) >> (clobber (scratch:SI)) >> ]) "j.C":50:38 discrim 1 155 {adddi3} >> (expr_list:REG_DEAD (reg:DI 73 [ _31 ]) >> (expr_list:REG_DEAD (reg/v:SI 41 [ __n ]) >> (nil)))) > > Not surprisingly we're focused on the subreg expression in there. > > The first checkpoint in my mind is IRA's allocation where we assign it > to reg 0. > > >> Popping a0(r41,l0) -- assign reg 0 > > > So given the use inside a paradoxical subreg, do we consider this valid? > > After the discussion from last week, I'm leaning a bit more towards no > than before. I thought it wasn't valid. AIUI, there are two mechanisms that try to prevent it: - valid_mode_changes_for_regno, which says which hard registers can form all subregs required by a pseudo. This is only used to restrict class choices though, rather than forbid individual registers. - This code in ira_build_conflicts: /* Now we deal with paradoxical subreg cases where certain registers cannot be accessed in the widest mode. */ machine_mode outer_mode = ALLOCNO_WMODE (a); machine_mode inner_mode = ALLOCNO_MODE (a); if (paradoxical_subreg_p (outer_mode, inner_mode)) { enum reg_class aclass = ALLOCNO_CLASS (a); for (int j = ira_class_hard_regs_num[aclass] - 1; j >= 0; --j) { int inner_regno = ira_class_hard_regs[aclass][j]; int outer_regno = simplify_subreg_regno (inner_regno, inner_mode, 0, outer_mode); if (outer_regno < 0 || !in_hard_reg_set_p (reg_class_contents[aclass], outer_mode, outer_regno)) { SET_HARD_REG_BIT (OBJECT_TOTAL_CONFLICT_HARD_REGS (obj), inner_regno); SET_HARD_REG_BIT (OBJECT_CONFLICT_HARD_REGS (obj), inner_regno); } } } which operates at the level of individual registers. So yeah, I think the first question is why ira_build_conflicts isn't kicking in for this register or (if it is) why we still get register 0. Thanks, Richard
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 +# <http://www.gnu.org/licenses/>. + +# 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 <typename _Tp, typename> using type = _Tp; +}; +template <bool, typename _If, typename _Else> +using __conditional_t = __conditional::type<_If, _Else>; +template <typename> constexpr bool is_lvalue_reference_v = true; +template <typename> bool is_same_v; +template <typename _From, typename _To> +constexpr bool is_convertible_v = __is_convertible(_From, _To); +template <typename> bool is_invocable_v; +template <typename...> using common_reference_t = int; +template <typename _Iterator> struct iterator_traits : _Iterator {}; +namespace __detail { +template <typename, typename _Up> +concept __same_as = is_same_v<_Up>; +} +template <typename _Tp, typename _Up> +concept same_as = __detail::__same_as<_Up, _Tp>; +template <typename _Derived, typename _Base> +concept derived_from = is_convertible_v<_Derived, _Base>; +template <typename, typename> +concept common_reference_with = + same_as<common_reference_t<>, common_reference_t<>>; +namespace __detail { +template <typename _Tp> +concept __boolean_testable = requires(_Tp __t) { __t; }; +template <typename _Tp, typename> +concept __weakly_eq_cmp_with = requires(_Tp __t) { __t; }; +} // namespace __detail +template <typename... _Args> +concept invocable = is_invocable_v<_Args...>; +template <typename> +concept regular_invocable = invocable<>; +template <typename _Fn> +concept predicate = __detail::__boolean_testable<_Fn>; +template <typename _Rel, typename, typename> +concept relation = predicate<_Rel>; +template <typename _Rel, typename _Tp, typename _Up> +concept strict_weak_order = relation<_Rel, _Tp, _Up>; +namespace { +struct duration { + duration() = default; + template <typename _Rep2> 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 <typename> using sys_time = time_point; +} // namespace +template <typename> class allocator; +namespace { +struct less {}; +} // namespace +struct forward_iterator_tag {}; +namespace __detail { +template <typename _Iter> struct __iter_traits_impl { + using type = iterator_traits<_Iter>; +}; +template <typename _Iter> using __iter_traits = __iter_traits_impl<_Iter>::type; +template <typename> struct __iter_concept_impl; +template <typename _Iter> + requires requires { typename _Iter; } +struct __iter_concept_impl<_Iter> { + using type = __iter_traits<_Iter>::iterator_concept; +}; +template <typename _Iter> +using __iter_concept = __iter_concept_impl<_Iter>::type; +template <typename _In> +concept __indirectly_readable_impl = common_reference_with<_In, _In>; +} // namespace __detail +template <typename _In> +concept indirectly_readable = __detail::__indirectly_readable_impl<_In>; +template <typename _Iter> +concept input_or_output_iterator = requires(_Iter __i) { __i; }; +template <typename _Sent, typename _Iter> +concept sentinel_for = __detail::__weakly_eq_cmp_with<_Sent, _Iter>; +template <typename _Iter> +concept forward_iterator = + derived_from<__detail::__iter_concept<_Iter>, forward_iterator_tag>; +template <typename _Fn, typename> +concept indirectly_regular_unary_invocable = regular_invocable<_Fn>; +template <typename _Fn, typename _I1, typename _I2> +concept indirect_strict_weak_order = strict_weak_order<_Fn, _I1, _I2>; +namespace __detail { +template <typename, typename> struct __projected; +} +template <indirectly_readable _Iter, + indirectly_regular_unary_invocable<_Iter> _Proj> +using projected = __detail::__projected<_Iter, _Proj>; +namespace ranges::__access { +template <typename _Tp> auto __begin(_Tp __t) { return __t.begin(); } +} // namespace ranges::__access +namespace __detail { +template <typename _Tp> +using __range_iter_t = decltype(ranges::__access::__begin(_Tp())); +} +template <typename _Tp> struct iterator_traits<_Tp *> { + typedef forward_iterator_tag iterator_concept; + using reference = _Tp; +}; +} // namespace std +template <typename _Iterator, typename> struct __normal_iterator { + using iterator_concept = std::__detail::__iter_concept<_Iterator>; + std::iterator_traits<_Iterator>::reference operator*(); + void operator++(); +}; +template <typename _IteratorL, typename _IteratorR, typename _Container> +bool operator==(__normal_iterator<_IteratorL, _Container>, + __normal_iterator<_IteratorR, _Container>); +int _M_get_sys_info_ri; +namespace std { +template <typename> struct allocator_traits; +template <typename _Tp> struct allocator_traits<allocator<_Tp>> { + using pointer = _Tp *; +}; +namespace { +namespace __detail { +template <typename _Tp> +concept __maybe_borrowed_range = is_lvalue_reference_v<_Tp>; +} +int end; +template <typename> +concept range = requires { end; }; +template <typename _Tp> +concept borrowed_range = __detail::__maybe_borrowed_range<_Tp>; +template <typename _Tp> using iterator_t = std::__detail::__range_iter_t<_Tp>; +template <typename _Tp> +concept forward_range = forward_iterator<iterator_t<_Tp>>; +struct dangling; +template <input_or_output_iterator _It, sentinel_for<_It> _Sent = _It> +struct subrange { + _It begin(); + _Sent end(); +}; +template <range _Range> +using borrowed_subrange_t = + __conditional_t<borrowed_range<_Range>, subrange<iterator_t<_Range>>, + dangling>; +} // namespace +template <typename _Alloc> struct _Vector_base { + typedef allocator_traits<_Alloc>::pointer pointer; +}; +template <typename _Tp, typename _Alloc = allocator<_Tp>> struct vector { + __normal_iterator<typename _Vector_base<_Alloc>::pointer, int> begin(); +}; +template <typename _Tp> 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<long> __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<Rule> rules; +}; +char name; +struct Rule { + int from; + void start_time(int, long) { + year_month_day ymd; + sys_time<long> 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
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 <jlaw@ventanamicro.com> 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.