Reverted the "Patch: clear NEXT_INSN, PREV_INSN and JUMP_LABEL on deleted insns"

[Hans-Peter Nilsson <hans-peter dot nilsson at axis dot com>]

On request and on behalf of Laurynas Biveinis I reverted his
patch, as he was unable to do so himself in the weekend.  The
patch caused regressions for several ports and some broke when
building.  The patch was IIUC supposed to expose at most a few
simple bugs with simple fixes, but that's unfortunately not what
happened.

Here's one patch I needed, cc0-specific.  This was indeed the
trivial, foreseen kind.  (I'll apply this as obvious later.)
It's exposed by gcc.c-torture/execute/20000403-1.c at -O1 for
cris-elf (which had only about 9 regressions FWIW).

	* final.c (final_scan_insn): Save PREV_INSN of prev
	before applying delete_insn.


Index: final.c
===================================================================
--- final.c	(revision 128224)
+++ final.c	(working copy)
@@ -2536,7 +2536,7 @@
 	   needs to be reinserted.  */
 	if (template == 0)
 	  {
-	    rtx prev;
+	    rtx prev, pprev;
 
 	    gcc_assert (prev_nonnote_insn (insn) == last_ignored_compare);
 
@@ -2546,8 +2546,10 @@
 	       scope note or an EH note.  */
 	    for (prev = insn;
 		 prev != last_ignored_compare;
-		 prev = PREV_INSN (prev))
+		 prev = pprev)
 	      {
+		pprev = PREV_INSN (prev);
+
 		if (NOTE_P (prev))
 		  delete_insn (prev);	/* Use delete_note.  */
 	      }

Here's another, one which obviously is a horrendous and wrong
hack (but for the regressions, working), but which has no simple
solution AFAICT.  It's exposed by e.g. g++.old-deja/g++.eh/ia64-1.C
for cris-elf.

The second hunk might look complicated but is actually of the
same trivial kind as the above and IIRC mentioned in some PR,
but it's a red herring for cris-elf (the code doesn't trig once
the insn chain is corrected).

The badness comes from "prev" being deleted as the result of a
call to emit_reload_insns a few lines above the call to
subst_reloads seen in the diff.  Unfortunately, holding
PREV_INSN (prev) wouldn't be much better; there's nothing more
special with that, than with "prev", it too could be feeding
"insn" and nothing bars it from being deleted.  I haven't found
anything good and safe in the "chain" struct either; there
simply isn't a handle to 'the insn(s) strictly before and after
a supposed emitted (or deleted) sequence of reload insns for
"insn"'.  (Here is where you prove me wrong.)  Solving this kind
of problem is IMHO a little more than fits in the last 24h
before stage 3.

Now, "prev" and "next" really should point at the right insn
before and after "insn" and its reloads.  If it doesn't, we'd
here skip a few of them and emit incorrect EH notes; silently
emitting wrong code, not as easily detected as an ICE on NULL
access.

Index: reload1.c
===================================================================
--- reload1.c	(revision 128224)
+++ reload1.c	(working copy)
@@ -4147,6 +4147,18 @@
 		 and that we moved the structure into).  */
 	      subst_reloads (insn);
 
+	      /* If PREV or NEXT are deleted (because they fed insn and
+		 were eliminated as part of reloading), we've lost those
+		 handles and have to assume we can just start over with
+		 "new" ones; there is nowhere we can be sure of getting a
+		 handle to a previous/next insn that has *not* been
+		 deleted as a result of reloading.  */
+	      if (INSN_DELETED_P (prev))
+		prev = PREV_INSN (insn);
+
+	      if (INSN_DELETED_P (next))
+		next = NEXT_INSN (insn);
+
 	      /* Adjust the exception region notes for loads and stores.  */
 	      if (flag_non_call_exceptions && !CALL_P (insn))
 		fixup_eh_region_note (insn, prev, next);
@@ -4155,17 +4167,25 @@
 		 we have generated are valid.  If not, give an error
 		 and delete them.  */
 	      if (asm_noperands (PATTERN (insn)) >= 0)
-		for (p = NEXT_INSN (prev); p != next; p = NEXT_INSN (p))
-		  if (p != insn && INSN_P (p)
-		      && GET_CODE (PATTERN (p)) != USE
-		      && (recog_memoized (p) < 0
-			  || (extract_insn (p), ! constrain_operands (1))))
+		{
+		  rtx pp;
+
+		  for (p = NEXT_INSN (prev); p != next; p = pp)
 		    {
-		      error_for_asm (insn,
-				     "%<asm%> operand requires "
-				     "impossible reload");
-		      delete_insn (p);
+		      pp = NEXT_INSN (p);
+
+		      if (p != insn && INSN_P (p)
+			  && GET_CODE (PATTERN (p)) != USE
+			  && (recog_memoized (p) < 0
+			      || (extract_insn (p), ! constrain_operands (1))))
+			{
+			  error_for_asm (insn,
+					 "%<asm%> operand requires "
+					 "impossible reload");
+			  delete_insn (p);
+			}
 		    }
+		}
 	    }
 
 	  if (num_eliminable && chain->need_elim)


I believe I observed a more detectable wrong-code regression
that seemed to be the result of delayed-branch stopping early on
a NULL-terminated list, miscompiling several tests, for example
gcc.c-torture/execute/builtins/pr23484-chk.c for cris-elf.  I
haven't investigate further than seeing assembly differences
from 128223 that are the kind of transformations that reorg.c
do, but where it "kept" a moved insn from a conditional branch,
one that it should have deleted as part of the move (actually
change in pr23484-chk-lib.s).

I'm sure other architectures find other little gems.  FWIW I'm
not opposed to the patch as such once the issues have been
cleared, but I think much more caution is needed.  For example,
poison PREV_INSN and NEXT_INSN instead of just setting them to
NULL.  Yes, I know L.B. said he did that as part of development
of the patch, but I'd rather that poisoning be kept, activated
with ENABLE_CHECKING and then remaining bugs causing an easily
spottable ICE by dereferencing say (rtx) 1 and 2 than I'd have
wrong-code and silent performance regressions when *I* get
around to compile code with this kind of change.  There should
also IMHO be ample time to find out; well within stage 1 or 2.

This code is likely to spot bugs in *any* code traversing a list
of insns (QED), so unless all such lists are identified, the
whole range of architectures oddities should be tested:
delayed-slot, cc0 and other different representations, many/few
registers, (text and rtl) peepholes, with/without scheduling and
whatnot.  See backends.html.

brgds, H-P