Internal compiler error building libstdc++ for vax

Internal compiler error building libstdc++ for vax

[coypu]

Hi folks,

netbsd's copy of GCC differs enough that it fails elsewhere with
gcc-trunk, but the problematic code is upstream.

updating netbsd to gcc 6.4.0, I get an internal compiler error building
libstdc++. (Long version: http://gnats.netbsd.org/53039)

Short version:

test.cc: In function 'bool std::atomic_flag_test_and_set_explicit(std::__atomic_flag_base*, std::memory_order)':
test.cc:25:3: internal compiler error: in patch_jump_insn, at cfgrtl.c:1271

comment at cfgrtl.c:1271 suggests:
	  /* If the substitution doesn't succeed, die.  This can happen
	     if the back end emitted unrecognizable instructions or if
	     target is exit block on some arches.  */
	  if (!redirect_jump (as_a <rtx_jump_insn *> (insn),
			      block_label (new_bb), 0))
	    {
	      gcc_assert (new_bb == EXIT_BLOCK_PTR_FOR_FN (cfun));

so it's saying the backend is generating garbage.

(gdb) call debug_insn_slim(insn)
   12: {pc={(zero_extract([r23:SI],0x1,0x1)!=0)?L14:pc};zero_extract([r23:SI],0x1,0x1)=0x1;}

I've found that if I modify vax/builtins.md:
@@ -61,7 +67,7 @@
   label = gen_label_rtx ();
   emit_move_insn (operands[0], const1_rtx);
-  emit_jump_insn (gen_jbbssi<mode> (operands[1], const0_rtx, label, operands[1]));
+  //emit_jump_insn (gen_jbbssi<mode> (operands[1], const0_rtx, label, operands[1]));
   emit_move_insn (operands[0], const0_rtx);
   emit_label (label);
   DONE;

it "fixes" my internal compiler error. However, I'm not sure what is
wrong with gen_jbbssi<mode>.

My current strategy of "just changing things and seeing if they help /
comparing to 50 examples of nearly identical code" doesn't appear
to be working despite many attempts :-)

Please help, thanks.

Re: Internal compiler error building libstdc++ for vax

[coypu]

(updating)
krister found a better hack patch which explains what the problem is,
adding a useless move at the end of the instruction, so the label is
not the last instruction.

(And, in the problem code, the last instruction in the function.)

--- a/external/gpl3/gcc/dist/gcc/config/vax/builtins.md
+++ b/external/gpl3/gcc/dist/gcc/config/vax/builtins.md
@@ -70,6 +70,7 @@
   emit_jump_insn (gen_jbbssi<mode> (operands[1], const0_rtx, label,
operands[1]));
   emit_move_insn (operands[0], const0_rtx);
   emit_label (label);
+  emit_move_insn (operands[0], operands[0]);
   DONE;
 }")

Re: Internal compiler error building libstdc++ for vax

[Jeff Law]

On 03/19/2018 03:46 PM, coypu@sdf.org wrote:
> (updating)
> krister found a better hack patch which explains what the problem is,
> adding a useless move at the end of the instruction, so the label is
> not the last instruction.
> 
> (And, in the problem code, the last instruction in the function.)
> 
> --- a/external/gpl3/gcc/dist/gcc/config/vax/builtins.md
> +++ b/external/gpl3/gcc/dist/gcc/config/vax/builtins.md
> @@ -70,6 +70,7 @@
>    emit_jump_insn (gen_jbbssi<mode> (operands[1], const0_rtx, label,
> operands[1]));
>    emit_move_insn (operands[0], const0_rtx);
>    emit_label (label);
> +  emit_move_insn (operands[0], operands[0]);
>    DONE;
>  }")
> 
You're almost certainly not fixing the problem with that patch.  You're
just twiddling the generated IL in such a way that you're hiding the
real problem.

If I had to hazard a guess it looks like those jbs* patterns don't
accept a reversed pc/label_ref.  There may be other problems, that's the
most glaring one I see.

As an example of it done right, look at "*branch" and "*branch_reversed".

jeff

Re: Internal compiler error building libstdc++ for vax

[coypu]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

It turns out (all from krister, I am still totally lost) that it is not
failing for this specific reason in this case.

Rather, the attached patch from krister fixes it, saying that gcc
wants to change the label and then doesn't recognise the new insn
thinking the memory_operand predicate is not satisfied.
The new predicate is from rs6000.

In retrospect the most important thing to provide was the 4 line shell
script to reproduce the problem, I felt uneasy sharing that because it
is with netbsd's copy of GCC (which I know how to cross-build).

For the purpose of changing it to support a reversed pc/label_ref, I can
probably cargo-cult make it look like branch and make the square peg fit
in a round hole by a lot experimenting, but I don't understand the code
I have to be changing to do that.

There is this construct in the code that I don't understand why I want
to do anything like it, even if I can parse what the individual parts of
it mean:

     (set (zero_extract:SI (match_operand:QI 3 "memory_operand" "+0")
			   (const_int 1)
			   (match_dup 1))
	  (const_int 1))])]


[-- Attachment #2: vax.patch --]
[-- Type: text/plain, Size: 3093 bytes --]

diff --git a/external/gpl3/gcc/dist/gcc/config/vax/builtins.md b/external/gpl3/gcc/dist/gcc/config/vax/builtins.md
index 7be1179..5fb6da6 100644
--- a/external/gpl3/gcc/dist/gcc/config/vax/builtins.md
+++ b/external/gpl3/gcc/dist/gcc/config/vax/builtins.md
@@ -77,13 +77,13 @@
   [(parallel
     [(set (pc)
 	  (if_then_else
-	    (ne (zero_extract:SI (match_operand:QI 0 "memory_operand" "g")
+	    (ne (zero_extract:SI (match_operand:QI 0 "volatile_mem_operand" "g")
 				 (const_int 1)
 				 (match_operand:SI 1 "general_operand" "nrm"))
 		(const_int 0))
 	    (label_ref (match_operand 2 "" ""))
 	    (pc)))
-     (set (zero_extract:SI (match_operand:QI 3 "memory_operand" "+0")
+     (set (zero_extract:SI (match_operand:QI 3 "volatile_mem_operand" "+0")
 			   (const_int 1)
 			   (match_dup 1))
 	  (const_int 1))])]
@@ -94,13 +94,13 @@
   [(parallel
     [(set (pc)
 	  (if_then_else
-	    (ne (zero_extract:SI (match_operand:HI 0 "memory_operand" "Q")
+	    (ne (zero_extract:SI (match_operand:HI 0 "volatile_mem_operand" "Q")
 				 (const_int 1)
 				 (match_operand:SI 1 "general_operand" "nrm"))
 		(const_int 0))
 	    (label_ref (match_operand 2 "" ""))
 	    (pc)))
-     (set (zero_extract:SI (match_operand:HI 3 "memory_operand" "+0")
+     (set (zero_extract:SI (match_operand:HI 3 "volatile_mem_operand" "+0")
 			   (const_int 1)
 			   (match_dup 1))
 	  (const_int 1))])]
@@ -111,13 +111,13 @@
   [(parallel
     [(set (pc)
 	  (if_then_else
-	    (ne (zero_extract:SI (match_operand:SI 0 "memory_operand" "Q")
+	    (ne (zero_extract:SI (match_operand:SI 0 "volatile_mem_operand" "Q")
 				 (const_int 1)
 				 (match_operand:SI 1 "general_operand" "nrm"))
 		(const_int 0))
 	    (label_ref (match_operand 2 "" ""))
 	    (pc)))
-     (set (zero_extract:SI (match_operand:SI 3 "memory_operand" "+0")
+     (set (zero_extract:SI (match_operand:SI 3 "volatile_mem_operand" "+0")
 			   (const_int 1)
 			   (match_dup 1))
 	  (const_int 1))])]
diff --git a/external/gpl3/gcc/dist/gcc/config/vax/predicates.md b/external/gpl3/gcc/dist/gcc/config/vax/predicates.md
index 7344192..f68c3f4 100644
--- a/external/gpl3/gcc/dist/gcc/config/vax/predicates.md
+++ b/external/gpl3/gcc/dist/gcc/config/vax/predicates.md
@@ -109,3 +109,16 @@
    (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")))))
+
+;; Return 1 if the operand is in volatile memory.  Note that during the
+;; RTL generation phase, memory_operand does not return TRUE for volatile
+;; memory references.  So this function allows us to recognize volatile
+;; references where it's safe.
+(define_predicate "volatile_mem_operand"
+  (and (and (match_code "mem")
+	    (match_test "MEM_VOLATILE_P (op)"))
+       (if_then_else (match_test "reload_completed")
+         (match_operand 0 "memory_operand")
+         (if_then_else (match_test "reload_in_progress")
+	   (match_test "strict_memory_address_p (mode, XEXP (op, 0))")
+	   (match_test "memory_address_p (mode, XEXP (op, 0))")))))

Re: Internal compiler error building libstdc++ for vax

[Jeff Law]

On 04/02/2018 10:15 AM, coypu@sdf.org wrote:
> It turns out (all from krister, I am still totally lost) that it is not
> failing for this specific reason in this case.
> 
> Rather, the attached patch from krister fixes it, saying that gcc
> wants to change the label and then doesn't recognise the new insn
> thinking the memory_operand predicate is not satisfied.
> The new predicate is from rs6000.
> 
> In retrospect the most important thing to provide was the 4 line shell
> script to reproduce the problem, I felt uneasy sharing that because it
> is with netbsd's copy of GCC (which I know how to cross-build).
> 
> For the purpose of changing it to support a reversed pc/label_ref, I can
> probably cargo-cult make it look like branch and make the square peg fit
> in a round hole by a lot experimenting, but I don't understand the code
> I have to be changing to do that.
> 
> There is this construct in the code that I don't understand why I want
> to do anything like it, even if I can parse what the individual parts of
> it mean:
> 
>      (set (zero_extract:SI (match_operand:QI 3 "memory_operand" "+0")
> 			   (const_int 1)
> 			   (match_dup 1))
> 	  (const_int 1))])]
> 
Again, you're almost certainly just papering over hte problem.  There's
no reason I'm aware of for a zero_extract insn to be restricted to
volatile memory operands.   All you're doing is making the pattern not
match very often which happens to avoid the bug.

I think you need to go back to my earlier reply and read it carefully.
In particular, you need an insn where the label_ref and pc are swapped.

Jeff

Re: Internal compiler error building libstdc++ for vax

[Maciej W. Rozycki]

On Sun, 8 Apr 2018, Jeff Law wrote:

> I think you need to go back to my earlier reply and read it carefully.
> In particular, you need an insn where the label_ref and pc are swapped.

 Ouch, there are no reversed interlocked branch instructions in the VAX 
ISA, so these would have to branch around a jump.  Like say:

(define_insn "*jbbssisi_reversed"
  [(parallel
    [(set (pc)
	  (if_then_else
	    (ne (zero_extract:SI (match_operand:SI 0 "memory_operand" "Q")
				 (const_int 1)
				 (match_operand:SI 1 "general_operand" "nrm"))
		(const_int 0))
	    (pc))
	    (label_ref (match_operand 2 "" "")))
     (set (zero_extract:SI (match_operand:SI 3 "memory_operand" "+0")
			   (const_int 1)
			   (match_dup 1))
	  (const_int 1))])]
  ""
  "jbssi %1,%0,0f;jbr %l2;0:")

and likewise with the other modes.

 Is this what you have in mind?

  Maciej

Re: Internal compiler error building libstdc++ for vax

[Jeff Law]

On 04/10/2018 01:27 PM, Maciej W. Rozycki wrote:
> On Sun, 8 Apr 2018, Jeff Law wrote:
> 
>> I think you need to go back to my earlier reply and read it carefully.
>> In particular, you need an insn where the label_ref and pc are swapped.
> 
>  Ouch, there are no reversed interlocked branch instructions in the VAX 
> ISA, so these would have to branch around a jump.  Like say:
> 
> (define_insn "*jbbssisi_reversed"
>   [(parallel
>     [(set (pc)
> 	  (if_then_else
> 	    (ne (zero_extract:SI (match_operand:SI 0 "memory_operand" "Q")
> 				 (const_int 1)
> 				 (match_operand:SI 1 "general_operand" "nrm"))
> 		(const_int 0))
> 	    (pc))
> 	    (label_ref (match_operand 2 "" "")))
>      (set (zero_extract:SI (match_operand:SI 3 "memory_operand" "+0")
> 			   (const_int 1)
> 			   (match_dup 1))
> 	  (const_int 1))])]
>   ""
>   "jbssi %1,%0,0f;jbr %l2;0:")
> 
> and likewise with the other modes.
> 
>  Is this what you have in mind?
Yea.  Various parts of GCC assume that it can swap the pc/label_ref to
invert the branch.  At least that was the case in the past.  You could
always create the pattern with the inefficient jump-around-jump to
verify behavior.

Wandering through jump.c it appears that it now tries to validate the
changes as they occur.  So if creating the swapped pattern works, it
might be beneficial to see which pass creates the swapped version and
see if it can be twiddled to use validate_change or a similar API to
avoid the need for the swapped pattern.


Jeff