syncing the GCC vax port

syncing the GCC vax port

[coypu]

hi folks,

i was interesting in tackling some problems gcc netbsd/vax has.
it has some ICEs which are in reload phase. searching around, the answer
to that is "switch to LRA first". Now, I don't quite know what that is
yet, but I know I need to try to do it.

As an initial step, I need to sync the source code.
netbsd/vax has some outstanding work on GCC.

I've done this, and I can run programs built by this compiler:
http://coypu.sdf.org/gcc-9-vax.diff

(My tree has more detail on the changes done:
https://github.com/gcc-mirror/gcc/compare/master...coypoop:vax )

Matt Thomas (the GCC VAX maintainer) is the author of most of these
changes, I suspect he will not be very responsive by email.
Not being the author, I might not be able to answer all the questions,
but I can try my best.

How do I get this across? comments? straight to gcc-patches? :-)

I know Jeff Law did not like the change to builtins.md as being wrong. I
can omit them, I forgot about it until typing this email :)

caveat: had an ICE during reload in the build process, I hid it
under the rug with -O0.

Re: syncing the GCC vax port

[Paul Koning]

> On Mar 30, 2019, at 5:03 AM, coypu@sdf.org wrote:
> 
> hi folks,
> 
> i was interesting in tackling some problems gcc netbsd/vax has.
> it has some ICEs which are in reload phase. searching around, the answer
> to that is "switch to LRA first". Now, I don't quite know what that is
> yet, but I know I need to try to do it.

That's not quite the whole story.

The answer is (1) switch from CC0 to CCmode condition code handling, which enables (2) switch from Reload to LRA.

(1) requires actual work, not terribly hard but not entirely trivial.  (2) may take as little as switching the "use LRA" flag to "yes".

I did (1) as well as a tentative (2) for pdp11 last year.  It was reasonably straightforward thanks to a pile of help from Eric Botcazou and his gcc wiki articles on the subject.  You might find the pdp11 deltas for CCmode helpful as a source of ideas, since the two machines have a fair amount in common as far as condition codes goes.  At least for the integer ops (pdp11 has separate floating point conditions, vax doesn't).

	paul

Re: syncing the GCC vax port

[coypu]

On Sun, Mar 31, 2019 at 01:25:50PM -0400, Paul Koning wrote:
> 
> 
> > On Mar 30, 2019, at 5:03 AM, coypu@sdf.org wrote:
> > 
> > hi folks,
> > 
> > i was interesting in tackling some problems gcc netbsd/vax has.
> > it has some ICEs which are in reload phase. searching around, the answer
> > to that is "switch to LRA first". Now, I don't quite know what that is
> > yet, but I know I need to try to do it.
> 
> That's not quite the whole story.
> 
> The answer is (1) switch from CC0 to CCmode condition code handling, which enables (2) switch from Reload to LRA.
> 
> (1) requires actual work, not terribly hard but not entirely trivial.  (2) may take as little as switching the "use LRA" flag to "yes".
> 
> I did (1) as well as a tentative (2) for pdp11 last year.  It was reasonably straightforward thanks to a pile of help from Eric Botcazou and his gcc wiki articles on the subject.  You might find the pdp11 deltas for CCmode helpful as a source of ideas, since the two machines have a fair amount in common as far as condition codes goes.  At least for the integer ops (pdp11 has separate floating point conditions, vax doesn't).
> 
> 	paul
> 

Hi paul!

I have been reading on this, so now I have a draft that compiles the
world's simplest C code (and nothing more, it will crash), but using
CCmode (I think).

I am being inspired by your port (which is a good thing since I know I
can ask questions about it :))

https://github.com/coypoop/gcc/commit/df135c019de33950c9997fdea3ce07c5c920384d

(I know that it's wrong!)

Re: syncing the GCC vax port

[Jeff Law]

On 3/31/19 11:25 AM, Paul Koning wrote:
> 
> 
>> On Mar 30, 2019, at 5:03 AM, coypu@sdf.org wrote:
>>
>> hi folks,
>>
>> i was interesting in tackling some problems gcc netbsd/vax has.
>> it has some ICEs which are in reload phase. searching around, the answer
>> to that is "switch to LRA first". Now, I don't quite know what that is
>> yet, but I know I need to try to do it.
> 
> That's not quite the whole story.
> 
> The answer is (1) switch from CC0 to CCmode condition code handling, which enables (2) switch from Reload to LRA.
> 
> (1) requires actual work, not terribly hard but not entirely trivial.  (2) may take as little as switching the "use LRA" flag to "yes".
> 
> I did (1) as well as a tentative (2) for pdp11 last year.  It was reasonably straightforward thanks to a pile of help from Eric Botcazou and his gcc wiki articles on the subject.  You might find the pdp11 deltas for CCmode helpful as a source of ideas, since the two machines have a fair amount in common as far as condition codes goes.  At least for the integer ops (pdp11 has separate floating point conditions, vax doesn't).
Right.  Another port one could look at which recently went through this
transition is the v850.

Jeff

Re: syncing the GCC vax port

[Jeff Law]

On 3/30/19 3:03 AM, coypu@sdf.org wrote:
> hi folks,
> 
> i was interesting in tackling some problems gcc netbsd/vax has.
> it has some ICEs which are in reload phase. searching around, the answer
> to that is "switch to LRA first". Now, I don't quite know what that is
> yet, but I know I need to try to do it.
> 
> As an initial step, I need to sync the source code.
> netbsd/vax has some outstanding work on GCC.
> 
> I've done this, and I can run programs built by this compiler:
> http://coypu.sdf.org/gcc-9-vax.diff
> 
> (My tree has more detail on the changes done:
> https://github.com/gcc-mirror/gcc/compare/master...coypoop:vax )
> 
> Matt Thomas (the GCC VAX maintainer) is the author of most of these
> changes, I suspect he will not be very responsive by email.
> Not being the author, I might not be able to answer all the questions,
> but I can try my best.
> 
> How do I get this across? comments? straight to gcc-patches? :-)
> 
> I know Jeff Law did not like the change to builtins.md as being wrong. I
> can omit them, I forgot about it until typing this email :)
> 
> caveat: had an ICE during reload in the build process, I hid it
> under the rug with -O0.
I don't recall what I objected to :-)  From looking at the changes I'd
be concerned about the changes from memory_operand to
volatile_mem_operand.  Without knowing more about the problem you're
trying to solve it's hard to ACK something like this.

The new "condjump" expander seems a bit under-specified.  Is there some
reason why you needed that expander and couldn't just add a name to a
existing define_insn?

A nit.  In that expander you have "gen_rtx_NE(VOIDmode ..."  There
should be a space between the NE and the open parenthesis.  That kind of
nit is repeated several times.

The changes in constraints.md do not seem to change functionality at
all, just reordering the oprerands.  However our preferred style is
what's in there right now.

ival >= 0  is the right way   0 <= ival is the wrong way.


I noticed the patch turns off flag_dwarf2_cfi_asm.  Is there a reason
for that?  But yet you create DEF_CFA notes in the prologue.  Is it the
case that the CFI bits just aren't ready for consumption yet?  If not,
then it may be easier to just not include those changes right now.


I can't really comment on the changes to how addreses are handled in
print_operand_address.  I'd just have to assume they're correct.

I don't know if we generally allow debug_rtx calls like are added.
Usually we gcc_assert or gcc_unreachable.

mkrtx needs a function comment and looks like its got some formatting
problems (spaces vs tabs issues and missing space between function name
an the open paren for arguments).  Similarly for vax_output_movmemsi and
legitimate_pic_operand_p, vax_decomposed_dimode_operand_p,

You've got undocumented #ifdefs in legitimate_pic_operand_p.

You've got incorrect operand ordering in the adjacent_operands_p changes.


The netbsd specific changes to the zero_extract patterns looks strange.
 Why why not just have the right operand.  And why change it in the
first place?

Those peepholes look strange.  Why is the first insn not just removed as
dead?

And if these changes were done by Matt Thomas, does he have a copyright
assignment on file?  If not, then we can't really use them.  These are
large enough that they'd require an assignment.

jeff

Re: syncing the GCC vax port

[coypu]

Sorry for the delay...
Updated diff: http://coypu.sdf.org/vax-gcc10.diff

On Mon, Apr 29, 2019 at 02:08:32PM -0600, Jeff Law wrote:
> On 3/30/19 3:03 AM, coypu@sdf.org wrote:
> > hi folks,
> > 
> > i was interesting in tackling some problems gcc netbsd/vax has.
> > it has some ICEs which are in reload phase. searching around, the answer
> > to that is "switch to LRA first". Now, I don't quite know what that is
> > yet, but I know I need to try to do it.
> > 
> > As an initial step, I need to sync the source code.
> > netbsd/vax has some outstanding work on GCC.
> > 
> > I've done this, and I can run programs built by this compiler:
> > http://coypu.sdf.org/gcc-9-vax.diff
> > 
> > (My tree has more detail on the changes done:
> > https://github.com/gcc-mirror/gcc/compare/master...coypoop:vax )
> > 
> > Matt Thomas (the GCC VAX maintainer) is the author of most of these
> > changes, I suspect he will not be very responsive by email.
> > Not being the author, I might not be able to answer all the questions,
> > but I can try my best.
> > 
> > How do I get this across? comments? straight to gcc-patches? :-)
> > 
> > I know Jeff Law did not like the change to builtins.md as being wrong. I
> > can omit them, I forgot about it until typing this email :)
> > 
> > caveat: had an ICE during reload in the build process, I hid it
> > under the rug with -O0.
> I don't recall what I objected to :-)  From looking at the changes I'd
> be concerned about the changes from memory_operand to
> volatile_mem_operand.  Without knowing more about the problem you're
> trying to solve it's hard to ACK something like this.

Removed from the diff. Unfortunately this introduces an ICE during the
build of GCC (but that will be the second kind)

> The new "condjump" expander seems a bit under-specified.  Is there some
> reason why you needed that expander and couldn't just add a name to a
> existing define_insn?

I didn't find an answer for this yet. What I got so far:

Apparently vax did not include builtins.md before, and so an adjustment
that should have happened when the cond-optab branch was merged didn't
happen.

> A nit.  In that expander you have "gen_rtx_NE(VOIDmode ..."  There
> should be a space between the NE and the open parenthesis.  That kind of
> nit is repeated several times.

Fixed.

> The changes in constraints.md do not seem to change functionality at
> all, just reordering the oprerands.  However our preferred style is
> what's in there right now.

Undid that.

> ival >= 0  is the right way   0 <= ival is the wrong way.

Fixed.

> I noticed the patch turns off flag_dwarf2_cfi_asm.  Is there a reason
> for that?  But yet you create DEF_CFA notes in the prologue.  Is it the
> case that the CFI bits just aren't ready for consumption yet?  If not,
> then it may be easier to just not include those changes right now.

This is due to a binutils issue. It looks similar to
https://github.com/riscv/riscv-binutils-gdb/issues/76
I have yet to find a fix for it.

> I can't really comment on the changes to how addreses are handled in
> print_operand_address.  I'd just have to assume they're correct.
> 
> I don't know if we generally allow debug_rtx calls like are added.
> Usually we gcc_assert or gcc_unreachable.

OK. I removed those and added a single gcc_unreachable.

> mkrtx needs a function comment and looks like its got some formatting
> problems (spaces vs tabs issues and missing space between function name
> an the open paren for arguments).  Similarly for vax_output_movmemsi and
> legitimate_pic_operand_p, vax_decomposed_dimode_operand_p,

Added comments.

> You've got undocumented #ifdefs in legitimate_pic_operand_p.

elf.h contains:

/* Don't allow *foo which foo is non-local */
#define NO_EXTERNAL_INDIRECT_ADDRESS

Is this sufficient? thanks.

> You've got incorrect operand ordering in the adjacent_operands_p changes.

Fixed.

> The netbsd specific changes to the zero_extract patterns looks strange.
>  Why why not just have the right operand.  And why change it in the
> first place?

Refers to same as first comment: I removed these changes. Back to the
drawing board with that crash.

> Those peepholes look strange.  Why is the first insn not just removed as
> dead?

I don't understand this comment (sorry). Can you clarify it?
I removed one peephole that was (0 &&...), if that helps.

> And if these changes were done by Matt Thomas, does he have a copyright
> assignment on file?  If not, then we can't really use them.  These are
> large enough that they'd require an assignment.

Yes, Matt Thomas has a copyright assignment on file.

Re: syncing the GCC vax port, atomic issue

[coypu]

On Fri, Sep 20, 2019 at 11:15:32AM +0000, coypu@sdf.org wrote:
> Removed from the diff. Unfortunately this introduces an ICE during the
> build of GCC...

I took another look at the VAX atomic pattern issue.
(http://gnats.netbsd.org/53039)
It is a compiler crash to do with the added atomic builtins.

The original suggestion was to introduce a reversed pattern, with
label / pc swapped. It did not work, unfortunately.

The crash backtrace is (gcc-trunk line numbers):
during RTL pass: expand
/home/fly/gcc/libatomic/flag.c: In function 'atomic_flag_test_and_set':
/home/fly/gcc/libatomic/flag.c:36:1: internal compiler error: in patch_jump_insn, at cfgrtl.c:1291
   36 | }
      | ^
0x718c22 patch_jump_insn
        /home/fly/gcc/gcc/cfgrtl.c:1290
0x718d2f redirect_branch_edge
        /home/fly/gcc/gcc/cfgrtl.c:1317
0x71b8c2 rtl_redirect_edge_and_branch
        /home/fly/gcc/gcc/cfgrtl.c:1450
0x703909 redirect_edge_and_branch(edge_def*, basic_block_def*)
        /home/fly/gcc/gcc/cfghooks.c:373
0x119ec6e try_forward_edges
        /home/fly/gcc/gcc/cfgcleanup.c:558
0x11a26ca try_optimize_cfg
        /home/fly/gcc/gcc/cfgcleanup.c:2961
0x11a26ca cleanup_cfg(int)
        /home/fly/gcc/gcc/cfgcleanup.c:3175
0x700a41 execute
        /home/fly/gcc/gcc/cfgexpand.c:6683


This seems to be where GCC has some logic specific to optimizing jumps.
Since this isn't a normal jump possible to eliminate by being clever,
but rather our only way of doing atomics, my gut feeling is that I
should avoid this jump optimizing code entirely.

Not telling GCC it's a jump worth optimizing seems to avoid the crash,
i.e.:

-  emit_jump_insn (gen_jbbssi<mode> (operands[1], const0_rtx, label, operands[1]));
+  emit_insn (gen_jbbssi<mode> (operands[1], const0_rtx, label, operands[1]));
+  LABEL_NUSES (label)++;

GCC builds, but when building netbsd I get an undefined reference in
libstdc++:
libstdc++.so: undefined reference to `.L72'
libstdc++.so: undefined reference to `.L75'

I wonder whether this is a right approach with a slightly off method.

Re: syncing the GCC vax port, atomic issue

[Jeff Law]

On 9/20/19 3:04 PM, coypu@sdf.org wrote:
> On Fri, Sep 20, 2019 at 11:15:32AM +0000, coypu@sdf.org wrote:
>> Removed from the diff. Unfortunately this introduces an ICE during the
>> build of GCC...
> 
> I took another look at the VAX atomic pattern issue.
> (http://gnats.netbsd.org/53039)
> It is a compiler crash to do with the added atomic builtins.
> 
> The original suggestion was to introduce a reversed pattern, with
> label / pc swapped. It did not work, unfortunately.
> 
> The crash backtrace is (gcc-trunk line numbers):
> during RTL pass: expand
> /home/fly/gcc/libatomic/flag.c: In function 'atomic_flag_test_and_set':
> /home/fly/gcc/libatomic/flag.c:36:1: internal compiler error: in patch_jump_insn, at cfgrtl.c:1291
>    36 | }
>       | ^
> 0x718c22 patch_jump_insn
>         /home/fly/gcc/gcc/cfgrtl.c:1290
> 0x718d2f redirect_branch_edge
>         /home/fly/gcc/gcc/cfgrtl.c:1317
> 0x71b8c2 rtl_redirect_edge_and_branch
>         /home/fly/gcc/gcc/cfgrtl.c:1450
> 0x703909 redirect_edge_and_branch(edge_def*, basic_block_def*)
>         /home/fly/gcc/gcc/cfghooks.c:373
> 0x119ec6e try_forward_edges
>         /home/fly/gcc/gcc/cfgcleanup.c:558
> 0x11a26ca try_optimize_cfg
>         /home/fly/gcc/gcc/cfgcleanup.c:2961
> 0x11a26ca cleanup_cfg(int)
>         /home/fly/gcc/gcc/cfgcleanup.c:3175
> 0x700a41 execute
>         /home/fly/gcc/gcc/cfgexpand.c:6683
> 
> 
> This seems to be where GCC has some logic specific to optimizing jumps.
> Since this isn't a normal jump possible to eliminate by being clever,
> but rather our only way of doing atomics, my gut feeling is that I
> should avoid this jump optimizing code entirely.
> 
> Not telling GCC it's a jump worth optimizing seems to avoid the crash,
> i.e.:
> 
> -  emit_jump_insn (gen_jbbssi<mode> (operands[1], const0_rtx, label, operands[1]));
> +  emit_insn (gen_jbbssi<mode> (operands[1], const0_rtx, label, operands[1]));
> +  LABEL_NUSES (label)++;
> 
> GCC builds, but when building netbsd I get an undefined reference in
> libstdc++:
> libstdc++.so: undefined reference to `.L72'
> libstdc++.so: undefined reference to `.L75'
> 
> I wonder whether this is a right approach with a slightly off method.
Conditional branching patterns must support the label_ref and pc
operands in either position.  Everything else I've seen on this thread
is just working around that broken aspect of the builtins.md file.


(define_insn "jbbssiqi"
  [(parallel
    [(set (pc)
	  (if_then_else
	    (ne (zero_extract:SI (match_operand:QI 0 "memory_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")
			   (const_int 1)
			   (match_dup 1))
	  (const_int 1))])]
  ""
  "jbssi %1,%0,%l2")

Note the position of the (label_ref ...) and (pc) operand.  You have to
have a pattern where they are reversed as well.

As an example look at the branch and branch_reversed patterns in vax.md

jeff

> 

Re: syncing the GCC vax port, atomic issue

[coypu]

On Fri, Sep 20, 2019 at 03:45:46PM -0600, Jeff Law wrote:
> Conditional branching patterns must support the label_ref and pc
> operands in either position.  Everything else I've seen on this thread
> is just working around that broken aspect of the builtins.md file.
> 
> 
> (define_insn "jbbssiqi"
>   [(parallel
>     [(set (pc)
> 	  (if_then_else
> 	    (ne (zero_extract:SI (match_operand:QI 0 "memory_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")
> 			   (const_int 1)
> 			   (match_dup 1))
> 	  (const_int 1))])]
>   ""
>   "jbssi %1,%0,%l2")
> 
> Note the position of the (label_ref ...) and (pc) operand.  You have to
> have a pattern where they are reversed as well.
> 
> As an example look at the branch and branch_reversed patterns in vax.md
> 
> jeff

Hi Jeff,

Thanks for the advice.
Introducing the reversed jbb* patterns doesn't seem to help with the
original issue. It crashes building libatomic.
I've attempted the following diff, as suggested by Maciej W. Rozycki.

(This isn't failing in the latest GCC because it doesn't include
vax/builtins.md at all)

> Ouch, there are no reversed interlocked branch instructions in the VAX
> ISA, so these would have to branch around a jump.
... https://gcc.gnu.org/ml/gcc/2018-04/msg00073.html

Is it the intended diff? did I get something wrong?

diff --git a/gcc/config/vax/builtins.md b/gcc/config/vax/builtins.md
index 2261467..db8ddc40 100644
--- a/gcc/config/vax/builtins.md
+++ b/gcc/config/vax/builtins.md
@@ -90,6 +90,24 @@
   ""
   "jbssi %1,%0,%l2")
 
+(define_insn "jbbssiqi_reversed"
+  [(parallel
+    [(set (pc)
+	  (if_then_else
+	    (ne (zero_extract:SI (match_operand:QI 0 "memory_operand" "g")
+				 (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:QI 3 "memory_operand" "+0")
+			   (const_int 1)
+			   (match_dup 1))
+	  (const_int 1))])]
+  ""
+  "jbssi %1,%0,0f\;jbr %l2\;0:")
+
+
 (define_insn "jbbssihi"
   [(parallel
     [(set (pc)
@@ -107,6 +125,24 @@
   ""
   "jbssi %1,%0,%l2")
 
+(define_insn "jbbssihi_reversed"
+  [(parallel
+    [(set (pc)
+          (if_then_else
+            (ne (zero_extract:SI (match_operand:HI 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:HI 3 "memory_operand" "+0")
+                           (const_int 1)
+                           (match_dup 1))
+          (const_int 1))])]
+  ""
+  "jbssi %1,%0,0f\;jbr %l2\;0:")
+
+
 (define_insn "jbbssisi"
   [(parallel
     [(set (pc)
@@ -125,6 +161,25 @@
   "jbssi %1,%0,%l2")
 
 
+(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:")
+
+
+
 (define_expand "sync_lock_release<mode>"
   [(set (match_operand:VAXint 0 "memory_operand" "+m")
 	(unspec:VAXint [(match_operand:VAXint 1 "const_int_operand" "n")
@@ -162,6 +217,23 @@
   ""
   "jbcci %1,%0,%l2")
 
+(define_insn "jbbcciqi_reversed"
+  [(parallel
+    [(set (pc)
+	  (if_then_else
+	    (eq (zero_extract:SI (match_operand:QI 0 "memory_operand" "g")
+				 (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:QI 3 "memory_operand" "+0")
+			   (const_int 1)
+			   (match_dup 1))
+	  (const_int 0))])]
+  ""
+  "jbcci %1,%0,0f\; jbr %l2\;0:")
+
 (define_insn "jbbccihi"
   [(parallel
     [(set (pc)
@@ -179,6 +251,24 @@
   ""
   "jbcci %1,%0,%l2")
 
+
+(define_insn "jbbccihi_reversed"
+  [(parallel
+    [(set (pc)
+	  (if_then_else
+	    (eq (zero_extract:SI (match_operand:HI 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:HI 3 "memory_operand" "+0")
+			   (const_int 1)
+			   (match_dup 1))
+	  (const_int 0))])]
+  ""
+  "jbcci %1,%0,0f\; jbr %l2\;0:")
+
 (define_insn "jbbccisi"
   [(parallel
     [(set (pc)
@@ -195,3 +285,21 @@
 	  (const_int 0))])]
   ""
   "jbcci %1,%0,%l2")
+
+(define_insn "jbbccisi_reversed"
+  [(parallel
+    [(set (pc)
+          (if_then_else
+            (eq (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 0))])]
+  ""
+  "jbssi %1,%0,0f\;jbr %l2\;0:")
+

Re: syncing the GCC vax port, atomic issue

[coypu]

On Fri, Sep 20, 2019 at 10:07:59PM +0000, coypu@sdf.org wrote:
> Introducing the reversed jbb* patterns doesn't seem to help with the
> original issue. It crashes building libatomic.

My loose understanding of what is going on:
- GCC emits this atomic in expand.
- When cleaning up, it looks for optimizations.
- It decides this is a branch to another branch situation, so maybe
  can be improved.
- This fails to output an instruction for unrelated reasons.
- Hit an assert.

I don't think that we should be trying to combine regular branch +
atomic branch in very generic code.
My guess is that, if it didn't crash now, it might emit a different kind
of branch which loses the atomic qualities, and result in wrong code.


I tried to single-step GCC, and it might be trying entirely different
instruction patterns.
I'm not sure whether I should put a lot of trust in the line numbers
shown from .md files, but it's trying nonlocal_goto in vax.md.
In any case, nothing from builtins.md.

Re: syncing the GCC vax port, atomic issue

[Paul Koning]

> On Sep 20, 2019, at 9:18 PM, coypu@sdf.org wrote:
> 
> On Fri, Sep 20, 2019 at 10:07:59PM +0000, coypu@sdf.org wrote:
>> Introducing the reversed jbb* patterns doesn't seem to help with the
>> original issue. It crashes building libatomic.
> 
> My loose understanding of what is going on:
> - GCC emits this atomic in expand.
> - When cleaning up, it looks for optimizations.
> - It decides this is a branch to another branch situation, so maybe
>  can be improved.
> - This fails to output an instruction for unrelated reasons.
> - Hit an assert.
> 
> I don't think that we should be trying to combine regular branch +
> atomic branch in very generic code.
> My guess is that, if it didn't crash now, it might emit a different kind
> of branch which loses the atomic qualities, and result in wrong code.

Or it might leave the atomic branch, in a place where it isn't really wanted.

I wonder if this could be avoided by representing the atomic branch by an UNSPEC rather than by a branch, since it isn't a "normal branch" that GCC knows about.

	paul

Re: syncing the GCC vax port, atomic issue

[Jeff Law]

On 9/21/19 12:27 PM, Paul Koning wrote:
> 
> 
>> On Sep 20, 2019, at 9:18 PM, coypu@sdf.org wrote:
>>
>> On Fri, Sep 20, 2019 at 10:07:59PM +0000, coypu@sdf.org wrote:
>>> Introducing the reversed jbb* patterns doesn't seem to help with the
>>> original issue. It crashes building libatomic.
>>
>> My loose understanding of what is going on:
>> - GCC emits this atomic in expand.
>> - When cleaning up, it looks for optimizations.
>> - It decides this is a branch to another branch situation, so maybe
>>  can be improved.
>> - This fails to output an instruction for unrelated reasons.
>> - Hit an assert.
>>
>> I don't think that we should be trying to combine regular branch +
>> atomic branch in very generic code.
>> My guess is that, if it didn't crash now, it might emit a different kind
>> of branch which loses the atomic qualities, and result in wrong code.
> 
> Or it might leave the atomic branch, in a place where it isn't really wanted.
> 
> I wonder if this could be avoided by representing the atomic branch by an UNSPEC rather than by a branch, since it isn't a "normal branch" that GCC knows about.
If the atomic branch is special from an architectural standpoint, then
that may be advisable.

Jeff

Re: syncing the GCC vax port, atomic issue

[Jeff Law]

On 9/20/19 7:18 PM, coypu@sdf.org wrote:
> On Fri, Sep 20, 2019 at 10:07:59PM +0000, coypu@sdf.org wrote:
>> Introducing the reversed jbb* patterns doesn't seem to help with the
>> original issue. It crashes building libatomic.
> 
> My loose understanding of what is going on:
> - GCC emits this atomic in expand.
> - When cleaning up, it looks for optimizations.
> - It decides this is a branch to another branch situation, so maybe
>   can be improved.
> - This fails to output an instruction for unrelated reasons.
> - Hit an assert.
> 
> I don't think that we should be trying to combine regular branch +
> atomic branch in very generic code.
I can see how you might think that, but the way the RTL optimizers work,
particularly the combiner is it tries to smash together two or more
insns that are related by dataflow into a single insn (I'm
over-simplifying, but it's good enough for this discussion).

That's an inherent design decision for the combiner, it's not likely to
change.   If you want to prevent the combiner from doing that, you have
to use UNSPECs

Jeff

Re: syncing the GCC vax port, atomic issue

[Richard Earnshaw (lists)]

On 01/10/2019 20:43, Jeff Law wrote:
> On 9/20/19 7:18 PM, coypu@sdf.org wrote:
>> On Fri, Sep 20, 2019 at 10:07:59PM +0000, coypu@sdf.org wrote:
>>> Introducing the reversed jbb* patterns doesn't seem to help with the
>>> original issue. It crashes building libatomic.
>>
>> My loose understanding of what is going on:
>> - GCC emits this atomic in expand.
>> - When cleaning up, it looks for optimizations.
>> - It decides this is a branch to another branch situation, so maybe
>>    can be improved.
>> - This fails to output an instruction for unrelated reasons.
>> - Hit an assert.
>>
>> I don't think that we should be trying to combine regular branch +
>> atomic branch in very generic code.
> I can see how you might think that, but the way the RTL optimizers work,
> particularly the combiner is it tries to smash together two or more
> insns that are related by dataflow into a single insn (I'm
> over-simplifying, but it's good enough for this discussion).
> 
> That's an inherent design decision for the combiner, it's not likely to
> change.   If you want to prevent the combiner from doing that, you have
> to use UNSPECs
> 
> Jeff
> 

There are some target hooks in combine that might help here. 
targetm.cannot_copy_insn_p and targetm.legitimate_combined_insn.  The 
former is used more widely than just combine, so you might need to be 
careful that it doesn't adversely affect other optimizations.  The 
latter relies on spotting that the combined insn is doing something 
stupid even though it matches a pattern in the back-end.  So it may be 
that neither really solves your problem in this case.

R.

Re: syncing the GCC vax port, atomic issue

[Segher Boessenkool]

On Wed, Oct 02, 2019 at 10:39:23AM +0100, Richard Earnshaw (lists) wrote:
> There are some target hooks in combine that might help here. 
> targetm.cannot_copy_insn_p and targetm.legitimate_combined_insn.  The 
> former is used more widely than just combine, so you might need to be 
> careful that it doesn't adversely affect other optimizations.  The 
> latter relies on spotting that the combined insn is doing something 
> stupid even though it matches a pattern in the back-end.  So it may be 
> that neither really solves your problem in this case.

Well, if what you need to prevent here is two branches from being combined
into one, you need to prevent many more passes than just combine from doing
the same, so this hook won't really help you.

I don't quite see why the generic optimisers make something that ICEs
later here, but yeah UNSPECs can side-step this whole issue.


Segher