On Fri, 27 Nov 2020, Ulrich Weigand wrote: > > NB I find the reindentation resulting in `push_reload' awful, just as I > > do either version of the massive logical expression involved. Perhaps we > > could factor these out into `static inline' functions sometime, and then > > have them split into individual returns within? > > I'm generally hesitant to do a lot of changes to the reload code base > at this stage. The strategy rather is to move all remaining targets > over to LRA and then simply delete reload :-) > > Given that you're modernizing the vax target, I'm wondering if you > shouldn't rather go all the way and move it over to LRA as well, > then you wouldn't be affected by any remaining reload deficiencies. > (The major hurdle so far was that LRA doesn't support CC0, but it > looks like you're removing that anyway ...)
I considered moving to LRA, but decided to make one step at a time, especially given the number of issues the VAX port has been suffering from. For example there are cases evident from regression test failures where new pseudos are created past-reload. That would require tracking down, and I think switching to LRA would best be made with cleaner test results so as not to introduce another variable into the picture. So I would consider it GCC 12 material, so that we have an actual release with the VAX port converted to MODE_CC, but still using reload. I think it could make some backports easier too if NetBSD people wanted to do it. > > + && (strict_low > > + || (subreg_lowpart_p (in) > > + && (CONSTANT_P (SUBREG_REG (in)) > > + || GET_CODE (SUBREG_REG (in)) == PLUS > > + || strict_low > > If we do this, then that "|| strict_low" clause is now redundant. Oh! I noticed the redundancy (which was in the way of the extra condition I was about to add) and rewrote the expression so as to remove it, and looks like I then left the line in place. Maybe I didn't save the final change in the editor or something. Sigh! Thanks for spotting it. > > + && REGNO (SUBREG_REG (in)) >= FIRST_PSEUDO_REGISTER > > + && reg_equiv_mem (REGNO (SUBREG_REG (in))) > > + && (mode_dependent_address_p > > + (XEXP (reg_equiv_mem (REGNO (SUBREG_REG (in))), 0), > > + MEM_ADDR_SPACE (reg_equiv_mem (REGNO (SUBREG_REG (in))))))))) > > I guess this is not incorrect, but I'm wondering if it would be > *complete* -- there are other cases where reload replaces a pseudo > with a MEM even where reg_equiv_mem is null. I wasn't aware of that. What would be the usual scenario? > That said, if it fixes the test suite errors you're seeing, it would > probably be OK to go with just this minimal change -- unless we can > just move to LRA as mentioned above. I've looked through the test results and indeed these suspicious ICEs remain: .../gcc/testsuite/gcc.dg/pr83623.c:13:1: internal compiler error: in change_address_1, at emit-rtl.c:2275 .../gcc/testsuite/gcc.dg/torture/vshuf-main.inc:27:1: internal compiler error: in change_address_1, at emit-rtl.c:2275 corresponding to: FAIL: gcc.dg/pr83623.c (internal compiler error) FAIL: gcc.dg/pr83623.c (test for excess errors) FAIL: gcc.dg/torture/vshuf-v16qi.c -O2 (internal compiler error) FAIL: gcc.dg/torture/vshuf-v16qi.c -O2 (test for excess errors) I'll investigate. Maciej
