Re: [gcc r14-2248] MIPS: Add bitwise instructions for mips16e2

Re: [gcc r14-2248] MIPS: Add bitwise instructions for mips16e2

[Maciej W. Rozycki]

On Mon, 3 Jul 2023, YunQiang Su via Gcc-cvs wrote:

> https://gcc.gnu.org/g:42d6b905c454e8f1b59d9248465d62a489b64972
> 
> commit r14-2248-g42d6b905c454e8f1b59d9248465d62a489b64972
> Author: Jie Mei <jie.mei@oss.cipunited.com>
> 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