Re: Silly gcc bugs

["Maciej W. Rozycki" <macro%linux-mips.org@localhost>]

On Mon, 8 Feb 2016, Maciej W. Rozycki wrote:

> > This, by the way, is gcc 4.1.3 on NetBSD 6.1.5 since gcc 4.8, AFAIK, is not
> > yet producing code natively.
> > 
> > Even after fixing that, though, compilation fails in the same file:
> > 
> > gchecksum.c: In function 'sha512_transform':
> > gchecksum.c:1250: error: insn does not satisfy its constraints:
> > (insn 516 457 479 2 (set (reg:SI 0 %r0)
> >         (reg/f:SI 17 virtual-stack-vars)) 16 {movsi_2} (nil)
> >     (nil))
> > gchecksum.c:1250: internal compiler error: in reload_cse_simplify_operands, at
> > postreload.c:393
> > 
> > Ideas?
> 
>  This seems to pop up regularly and with a quick setup I've been able to 
> reproduce it with my VAX/Linux 4.1.2 compiler too, so I'll see if I can 
> give it a stab.

 As it is this is due to an issue in the virtual register instantiation 
pass, which should have eliminated references to `virtual-stack-vars' 
early on.  The problem is this insn (one of the few following the same 
pattern; I've chosen the simplest):

(insn 205 204 206 6 (set (reg:SI 150)
        (plus:SI (plus:SI (mult:SI (reg:SI 38 [ D.11363 ])
                    (const_int 8 [0x8]))
                (reg/f:SI 17 virtual-stack-vars))
            (const_int -896 [0xfffffc80]))) -1 (nil)
    (nil))

which is generated from the `movaddrdi' load address RTL pattern and uses 
the indexed addressing mode.

 The reference to the `virtual-stack-vars' virtual register within is 
supposed to be substituted with the frame pointer, offset appropriately, 
in `instantiate_virtual_regs_in_insn'.  But this requires operands to be 
extracted by matching to a known insn and there is none, so the virtual 
register falls through up until reload, at which point the compiler has no 
idea what to do about it and gives up in this spectacular manner.

 I've been able to correct this problem with the use of the patch below.  
Now the virtual register instantiation pass rewrites the insn above into:

(insn 444 204 205 6 (set (reg:SI 240)
        (plus:SI (reg/f:SI 13 %fp)
            (const_int -4 [0xfffffffc]))) -1 (nil)
    (nil))

(insn 205 444 206 6 (set (reg:SI 150)
        (plus:SI (plus:SI (mult:SI (reg:SI 38 [ D.11363 ])
                    (const_int 8 [0x8]))
                (reg:SI 240))
            (const_int -896 [0xfffffc80]))) 57 {*addsi4_mova} (nil)
    (nil))

which is not perfect as the two constants could have been combined, but I 
think it is reasonable and should unblock you now.  Going further would 
require special-casing this RTL in `instantiate_virtual_regs_in_insn' I 
believe; see the place commented with: "Handle a plus involving a virtual 
register by determining if the operands remain valid if they're modified 
in place" in function.c, if curious.  The patterns are needed anyway.

 Two patterns are required in case no immediate offset is requested to 
`virtual-stack-vars' in a reference.  In this case `*addsi4_mova' is one 
that matters and makes things work though.

 I've checked the generated assembly and I think it is correct although I 
can't easily verify it at the run time right now, so I'll appreciate if 
you try this change and let me know if the code that failed to build also 
works correctly.

 If it works for your case, then it would be nice to put it through full 
GCC regression testing too, however doing that in a cross-compilation 
environment is tricky and I'm not set up for native testing with the 
VAX/Linux port, it's simply too immature.  So if someone steps in and does 
that, then I'll appreciate it -- in a native environment it's as easy as 
running `make check' (or maybe `make -C gcc check' to exclude libraries 
from being tested in return for a quicker run) in your build tree.  

 You will need to have TCL and DejaGNU installed to run this testing, and 
maybe also Expect (I'm not sure -- GDB does need it, but I think GCC does 
not).  It will take several hours at the very least on slow hardware, 
maybe even days, so choose your fastest VAX and keep it on a UPS. ;)  

 Running in a cross-compilation environment would speed up things 
considerably as the target machine would be offloaded from compilations -- 
which would then run on the build machine, possibly several orders of 
magnitude faster -- and limited to running test test cases only, so this 
is something definitely to look into sometime.

 You'll need to run this twice, of course, with a vanilla GCC build and 
then again with this change applied, to be able to see -- as the name of 
this testing implies -- if anything has regressed (the first pass would 
not be needed if we had clean vanilla results, but I dare not expecting 
that to be the case).

 By the look of it upstream GCC head requires a change like this too.

 Let me know if you have any questions or comments.

2016-02-14  Maciej W. Rozycki  <macro%linux-mips.org@localhost>

	gcc/
	* config/vax/predicates.md (const_248_operand): New predicate.
	* config/vax/vax.md (*addsi3_mova, *addsi4_mova): New insns.

  Maciej

gcc-4.1.2-vax-mova.patch
Index: gcc-4.1.2/gcc/config/vax/predicates.md
===================================================================
--- gcc-4.1.2.orig/gcc/config/vax/predicates.md
+++ gcc-4.1.2/gcc/config/vax/predicates.md
@@ -102,3 +102,11 @@
    (and (match_code "const_int,const_double,subreg,reg,mem")
 	(and (match_operand:DI 0 "general_operand" "")
 	     (not (match_operand:DI 0 "illegal_addsub_di_memory_operand")))))
+
+;; Match 2, 4, or 8.  Used for mova multiplicands.
+(define_predicate "const_248_operand"
+   (match_code "const_int")
+{
+  HOST_WIDE_INT i = INTVAL (op);
+  return i == 2 || i == 4 || i == 8;
+})
Index: gcc-4.1.2/gcc/config/vax/vax.md
===================================================================
--- gcc-4.1.2.orig/gcc/config/vax/vax.md
+++ gcc-4.1.2/gcc/config/vax/vax.md
@@ -405,6 +405,54 @@
 		 (match_operand:DI 2 "general_operand" "Fsro,Fs")))]
   "!TARGET_QMATH"
   "* return vax_output_int_add (insn, operands, DImode);")
+
+;; Scaled additions using mova, needed for virtual register instantiation.
+(define_insn "*addsi3_mova"
+  [(set (match_operand:SI 0 "nonimmediate_operand" "=g")
+	(plus:SI (mult:SI (match_operand:SI 1 "register_operand" "r")
+			  (match_operand 2 "const_248_operand" "n"))
+		 (match_operand:SI 3 "register_operand" "r")))]
+  ""
+  "*
+{
+  int i = INTVAL (operands[2]);
+
+  switch (i)
+    {
+    case 2:
+      return \"movaw (%3)[%1],%0\";
+    case 4:
+      return \"moval (%3)[%1],%0\";
+    case 8:
+      return \"movaq (%3)[%1],%0\";
+    default:
+      gcc_unreachable ();
+    }
+}")
+
+(define_insn "*addsi4_mova"
+  [(set (match_operand:SI 0 "nonimmediate_operand" "=g")
+	(plus:SI (plus:SI (mult:SI (match_operand:SI 1 "register_operand" "r")
+				   (match_operand 2 "const_248_operand" "n"))
+			  (match_operand:SI 3 "register_operand" "r"))
+		 (match_operand:SI 4 "immediate_operand" "i")))]
+  ""
+  "*
+{
+  int i = INTVAL (operands[2]);
+
+  switch (i)
+    {
+    case 2:
+      return \"movaw %4(%3)[%1],%0\";
+    case 4:
+      return \"moval %4(%3)[%1],%0\";
+    case 8:
+      return \"movaq %4(%3)[%1],%0\";
+    default:
+      gcc_unreachable ();
+    }
+}")
 
 ;;- All kinds of subtract instructions.