More general patch for "mips offs > 16bit",(emit_insn/force_operand inconsistencies, expr.c 1.76).

Hans-Peter Nilsson hp@bitrange.com
Sun Feb 28 07:20:00 GMT 1999


It seems the nucleus of this bug is something quite similar to
what I saw and reported Nov 1996, namely forgetting to use
force_operand in places where it is needed.
 I reported this to the at-the-time quite non-responsive gcc2 and
g++@cygnus.com, and after pestering several people (but no lists ;-)
about where to turn, I got a reply from Mike Stump (hi Mike,
remember <199611281636.RAA29735.cygnus.gcc2@scar.axis.se>?)
saying I should follow up with a minimal set of patches.  But I
didn't, shame on me. :-(

It's kinda spooky, because I picked this bug-report at random to
play with, somehow hoping to make use of the fallout for some
debugging howto-introduction-guide or whatever.  (Sorry, I will
probably not follow through on that for any foreseeable time.)

End of anecdotal part...

So here we go again.  At that time (1996), there were constructs
like "emit_move_insn (return_val_rtx, plus_constant (temp, -8));",
(fixed since long it seems).  Now there are force_reg constructs
where an operand can be an address; similar to that above, but
not as obvious.  Since emit_move_insn is called with that
unmodified operand, the effect is just as bad, unless
optimization is on, doing transformations that make it
recognized.

I'm not completely sure what TRT is here.  At that time, I
thought that emit_move_insn should always be called with a valid
operand (that is, through force_operand if necessary).
 Now, I *started* thinking that emit_move_insn should take care
to make its input operand a valid operand.  At least the
comments for force_reg, force_not_mem and emit_move_insn made me
believe there was nothing invalid about giving them something
that is not a valid movP operand, like an address consisting of
(plus reg const_int).
 On the other hand, there's force_operand that does this job,
and is used in a lot of places just before calling
emit_move_insn or force_reg.  But again on the first hand, not
all uses of an asserted operand is to call emit_move_insn.  And
it seems better to fix the called function than all the callers
(even though *most* are correct as it is).

But unfortunately, adding a "y = force_operand (y, NULL_RTX);"
to the top of move_insn_1 (before or after the
protect_from_queue calls; I tried both) causes a sig11 (the
repeatable kind) when bootstrapping i686-pc-linux-gnulibc1,
compiling _floatdidf with -O2 -fPIC or something.  Looking at it
in gdb, it seems that the move_insn_1 change causes creative
re-use of an obstack...
 If this solution *still* seems right to those of you who really
get the internals, lightly described in the section "(RTL
memory) Sharing" in TFM (that is, if you're confused rather than
chuckling right now), then there might be another bug.

So, this patch fixes the offending constructs in
expand_assignment and expand_expr, introduced by the 1.76 change
in expr.c.  To wit, it is the first hunk of the patch that fixes
the mips lossage with e.g. mipsel-unknown-netbsd.  I tried adding
an abort in emit_move_insn_1 to trigger if the mov_optab
insn_operand_predicates did not match, but that triggered right
away during libgcc.a build in bootstrap.  I'll investigate if
that is considered the right way to go.
 By the way, those lines in the 1.76 change look strange; why
not call change_address (force_reg (force_operand (plus_constant ...))
right away?  Instead of <change_address (plus_constant ...) then
check if you got a (MEM reg), and if not, call change_address again>?
Was it that the missing-force_operand bug showed up in a more
obvious way?


Anyhow, this patch lets i686-pc-linux-gnulibc1 run "bootstrap
check" with no changes in test-results, and seems to solve the
mips problem (I only checked the test-cases) in
<URL: http://egcs.cygnus.com/ml/egcs-bugs/1999-02/msg00786.html >
and <URL: http://www.cygnus.com/ml/egcs-bugs/1998-Oct/0928.html >,
for both the release branch and the main trunk.

Sun Feb 28 12:18:37 1999  Hans-Peter Nilsson  <hp@bitrange.com>

	* expr.c (expand_assignment): Filter XEXP(,0) of result of
	change_address through force_operand before calling force_reg.
	(expand_expr): Ditto.

===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/expr.c,v
retrieving revision 1.128
diff -p -c -r1.128 expr.c
*** expr.c	1999/02/19 11:19:30	1.128
--- expr.c	1999/02/28 11:16:49
*************** expand_assignment (to, from, want_value,
*** 3290,3298 ****
  	      if (GET_CODE (XEXP (temp, 0)) == REG)
  	        to_rtx = temp;
  	      else
! 		to_rtx = change_address (to_rtx, mode1,
! 				         force_reg (GET_MODE (XEXP (temp, 0)),
! 						    XEXP (temp, 0)));
  	      bitpos = 0;
  	    }
  
--- 3290,3300 ----
  	      if (GET_CODE (XEXP (temp, 0)) == REG)
  	        to_rtx = temp;
  	      else
! 		to_rtx
! 		  = change_address (to_rtx, mode1,
! 				    force_reg (GET_MODE (XEXP (temp, 0)),
! 					       force_operand (XEXP (temp, 0),
! 							      NULL_RTX)));
  	      bitpos = 0;
  	    }
  
*************** expand_expr (exp, target, tmode, modifie
*** 6433,6439 ****
  		else
  		  op0 = change_address (op0, mode1,
  					force_reg (GET_MODE (XEXP (temp, 0)),
! 						   XEXP (temp, 0)));
  		bitpos = 0;
  	      }
  
--- 6435,6442 ----
  		else
  		  op0 = change_address (op0, mode1,
  					force_reg (GET_MODE (XEXP (temp, 0)),
! 						   force_operand (XEXP (temp, 0),
! 								  NULL_RTX)));
  		bitpos = 0;
  	      }
  
brgds, H-P



More information about the Gcc-patches mailing list