Re: [gcc r14-2248] MIPS: Add bitwise instructions for mips16e2
On Mon, 3 Jul 2023, YunQiang Su via Gcc-cvs wrote: > https://gcc.gnu.org/g:42d6b905c454e8f1b59d9248465d62a489b64972 > > commit r14-2248-g42d6b905c454e8f1b59d9248465d62a489b64972 > Author: Jie Mei > Date: Mon Jun 19 16:29:53 2023 +0800 > > MIPS: Add bitwise instructions for mips16e2 > > There are shortened bitwise instructions in the mips16e2 ASE, > for instance, ANDI, ORI/XORI, EXT, INS etc. . This change was committed without any public review and it breaks the build of the compiler for the `mipsel-linux-gnu' target: .../gcc/config/mips/mips-msa.md:435:26: error: 'and' of mutually exclusive equal-tests is always 0 [-Werror] 435 | DONE; .../gcc/config/mips/mips-msa.md:435:26: error: 'and' of mutually exclusive equal-tests is always 0 [-Werror] .../gcc/config/mips/mips.md:822:1: error: 'and' of mutually exclusive equal-tests is always 0 [-Werror] 822 | ;; conditional-move-type condition is needed. | ^ .../gcc/config/mips/mips.md:822:1: error: 'and' of mutually exclusive equal-tests is always 0 [-Werror] cc1plus: all warnings being treated as errors make[2]: *** [Makefile:2928: build/gencondmd.o] Error 1 Moreover the error messages indicate dead code has been introduced. This shows the lack of due dilligence in any review this patch series may have received off-list. At the very least the maintainer is supposed to verify that code builds before it has been committed. I volunteered to review this patch series for a reason, not only to ensure meeting the GNU Coding Standards, but because I spotted code design problems. I realise I did not make the review right away as I could not afford the time required for such a medium large sized patch series last month. This does not mean the changes should have been committed without verification or any review at all. I raised my concerns back when YunQiang volunteered to become a MIPS target maintainer. He was accepted by the GCC Steering Committee regardless, with a word of advice to be vigilant about GCC Coding Standards. I would expect a responsible person to follow such advice. Back when I became a MIPS target maintainer for binutils and GDB I worked for Imagination Technologies and I told my manager not to expect me to be more relaxed when reviewing changes submitted by my coworkers. He replied he actually expected me to be stricter about such changes, which was exactly the reply I expected. Here it seems to me the patch series was fast-tracked despite having been submitted from a person working for the same company. This leaves me with a bad feeling. Also from what I have seen written I have a feeling that the new maintainer has assumed the position of authority rather than duty. This doesn't feel right to me. Given the concerns I previously raised were in my reception not correctly addressed I have a feeling my voice has not been listened to. This makes me less than motivated to strive and find time for MIPS reviews, which I find a significant mental burden, especially when code is far below the standard expected. Therefore given the turn of events I am not going to review any further MIPS changes submitted and will limit myself to the VAX backend. Considering my continued interest in keeping my MIPS hardware alive I will probably regret that as things continue breaking and bad design decisions are propagated, but without the lack of support from the community I need to draw a line somewhere. NB it cost me disruption and half a working day lost in tracking down what has happened here and consequently I haven't completed what I meant to and which I expected to be a trivial task of rebuilding the compiler and trying it on a piece of code I wanted to check. Maciej
Re: LRA for avr: help with FP and elimination
On 7/13/23 05:27, SenthilKumar.Selvaraj--- via Gcc wrote:
Hi,
I've been spending some (spare) time checking what it would take to
make LRA work for the avr target.
Right after I removed the TARGET_LRA_P hook disabling LRA, building
libgcc failed with a weird ICE.
On the avr, the stack pointer (SP)
is not used to access stack slots
It is very uncommon target then.
- TARGET_CAN_ELIMINATE returns false
if frame_pointer_needed, and TARGET_FRAME_POINTER_REQUIRED returns true
if get_frame_size() > 0.
With LRA, however, reload generates
(insn 159 239 240 7 (set (mem/c:QI (plus:HI (reg/f:HI 32 __SP_L__)
(const_int 1 [0x1])) [2 %sfp+1 S1 A8])
(reg:QI 24 r24 [orig:86 a ] [86])) "case.c":7:7 86 {movqi_insn_split}
(nil))
and the backend code errors out when it finds SP is being used as a
pointer register.
Digging through the RTL dumps, I found the following. For the
following insn sequence in *.ira
(insn 189 128 159 7 (set (reg:HI 58 [ b ])
(const_int 0 [0])) "case.c":7:7 101 {*movhi_split}
(nil))
(insn 159 189 160 7 (set (subreg:QI (reg:HI 58 [ b ]) 0)
(reg:QI 86 [ a ])) "case.c":7:7 86 {movqi_insn_split}
(nil))
(insn 160 159 32 7 (set (subreg:QI (reg:HI 58 [ b ]) 1)
(reg:QI 87 [ a+1 ])) "case.c":7:7 86 {movqi_insn_split}
(nil))
1. For r58, IRA picks R28:R29, which is the frame pointer for avr.
Popping a13(r58,l0) -- assign reg 28
2. LRA sees the subreg in insn 159 and generates a reload reg
(r125). simplify_subreg_regno (lra-constraints.cc:1810) however
bails (returns -1) if the reg involved is FRAME_POINTER_REGNUM and
reload isn't completed yet. LRA therefore decides rclass for the
pseudo reg is NO_REGS.
Creating newreg=125 from oldreg=58, assigning class NO_REGS to subreg reg r125
159: r125:HI#0=r86:QI
4. As rclass is NO_REGS, LRA picks an insn alternative that involves memory.
That is my understanding, please correct me if I'm wrong.
0 Small class reload: reject+=3
0 Non input pseudo reload: reject++
Cycle danger: overall += LRA_MAX_REJECT
alt=0,overall=610,losers=1,rld_nregs=1
0 Small class reload: reject+=3
0 Non input pseudo reload: reject++
alt=1: Bad operand -- refuse
0 Non pseudo reload: reject++
alt=2,overall=1,losers=0,rld_nregs=0
Choosing alt 2 in insn 159: (0) Qm (1) rY00 {movqi_insn_split}
5. LRA creates stack slots, and then uses the FP register to access
the slots. This is despite r58 already being assigned R28:R29.
6. TARGET_FRAME_POINTER_REQUIRED is never called, and therefore
frame_pointer_needed is not set, despite the creation of stack
slots. TARGET_CAN_ELIMINATE therefore okays elimination of FP to SP,
and this eventually causes the ICE when the avr backend sees SP being
used as a pointer register.
This is the relevant sequence after reload
(insn 189 128 239 7 (set (reg:HI 28 r28 [orig:58 b ] [58])
(const_int 0 [0])) "case.c":7:7 101 {*movhi_split}
(nil))
(insn 239 189 159 7 (set (mem/c:HI (plus:HI (reg/f:HI 32 __SP_L__)
(const_int 1 [0x1])) [2 %sfp+1 S2 A8])
(reg:HI 28 r28 [orig:58 b ] [58])) "case.c":7:7 101 {*movhi_split}
(nil))
(insn 159 239 240 7 (set (mem/c:QI (plus:HI (reg/f:HI 32 __SP_L__)
(const_int 1 [0x1])) [2 %sfp+1 S1 A8])
(reg:QI 24 r24 [orig:86 a ] [86])) "case.c":7:7 86 {movqi_insn_split}
(nil))
(insn 240 159 241 7 (set (reg:HI 28 r28 [orig:58 b ] [58])
(mem/c:HI (plus:HI (reg/f:HI 32 __SP_L__)
(const_int 1 [0x1])) [2 %sfp+1 S2 A8])) "case.c":7:7 101
{*movhi_split}
(nil))
(insn 241 240 160 7 (set (mem/c:HI (plus:HI (reg/f:HI 32 __SP_L__)
(const_int 1 [0x1])) [2 %sfp+1 S2 A8])
(reg:HI 28 r28 [orig:58 b ] [58])) "case.c":7:7 101 {*movhi_split}
(nil))
(insn 160 241 242 7 (set (mem/c:QI (plus:HI (reg/f:HI 32 __SP_L__)
(const_int 2 [0x2])) [2 %sfp+2 S1 A8])
(reg:QI 18 r18 [orig:87 a+1 ] [87])) "case.c":7:7 86 {movqi_insn_split}
(nil))
(insn 242 160 33 7 (set (reg:HI 28 r28 [orig:58 b ] [58])
(mem/c:HI (plus:HI (reg/f:HI 32 __SP_L__)
(const_int 1 [0x1])) [2 %sfp+1 S2 A8])) "case.c":7:7 101
{*movhi_split}
(nil))
For choices other than FP, simplify_subreg_regno returns the correct part
of the wider HImode reg, so rclass is not NO_REGS, and things workout fine.
I checked what classic reload does in the same situation - it picks a
different register (R25) instead of spilling to a stack slot.
(insn 189 128 159 7 (set (reg:HI 28 r28 [orig:58 b ] [58])
(const_int 0 [0])) "case.c":7:7 101 {*movhi_split}
(nil))
(insn 159 189 226 7 (set (reg:QI 25 r25)
(reg:QI 24 r24 [orig:86 a ] [86])) "case.c":7:7 86 {movqi
gcc-12-20230714 is now available
Snapshot gcc-12-20230714 is now available on https://gcc.gnu.org/pub/gcc/snapshots/12-20230714/ and on various mirrors, see http://gcc.gnu.org/mirrors.html for details. This snapshot has been generated from the GCC 12 git branch with the following options: git://gcc.gnu.org/git/gcc.git branch releases/gcc-12 revision 995c717500c368c5aec7889dfa047cff7cb0139b You'll find: gcc-12-20230714.tar.xz Complete GCC SHA256=8b0060164a55d0b836d3750918ececb1f75bfe17dfd1c367c76b217b16bb037c SHA1=935daf1376c0bc6f2ade4c5262d3359735f15ac5 Diffs from 12-20230707 are available in the diffs/ subdirectory. When a particular snapshot is ready for public consumption the LATEST-12 link is updated and a message is sent to the gcc list. Please do not use a snapshot before it has been announced that way.
