🚀 go-pugleaf

Abstract:
Code vanishes when optimizing a float => long long value.  This is due
to a misplaced "clobber" generated in expr.c; see tail of letter for a
pair of fixes.  By comparing the end of optabs.c:expand_fix() and
optabs.c:expand_float() this fix should be obvious.  These fixes can
not possibly break anything.


In gcc-2.7.2, code vanishes for mode-sizes larger than what can be
moved by a single instruction, when converting a float-mode to an
integer mode.

This can happen to any target that uses sizes larger than it can
handle in a single instruction, and with any optimization -O1 -O2 -O3,
with or without using a frame pointer.

Sorry; I cannot give you reasonable enough information to reproduce
exactly this case.  I'm updating our port of gcc to version 2.7.2 for
a proprietary embedded architecture (called CRIS, feel free to request
information).
 Asking any of you bug-gcc-maintainers to reproduce this bug would
involve sending a total set of tm.h tm.c and md, maybe also a cpu
description, a simulator, and how-to set it all up to execute the same
code. 
 Not that this is a "proprietary-code-problem", it's just the code
size and the amount of work for you.  If you want to look at it, I can
send it to you.

Hopefully you accept the description and the study that follows.  I've
done my best to make the description follow the guidelines in the
gcc.info-node "Bugs:Sending Patches".

 You should, however, be able to reproduce this problem with any
target that fulfills these criterias:

* No machine description for an instruction to move a long-long (movdi),
  so it has to be done by gcc in SI-parts (other instructions for
  DImode may exist, such as cmpdi2).
* No machine description for an instruction to convert a long-double 
  (any mode; SF, DF, TF etc.) to a DI, "floatdisf", so it has to be
  done by a library call.
* No machine description for an instruction to convert a long-long (DI)
  to a "long double"; "fixsfdi", so it has to be done by a library call.

...and otherwise a general architecture (m68k-like, but a more general
 class of registers, simpler addressing modes etc.)


So, here is the code that causes the trouble. It is the SECOND abort()
that gets executed.  Note: This is from Torbjorn Granlunds
c-regression-test-suite, "c-torture-1.39/execute/930622-2.c"

-----------------------v
long double
ll_to_ld (long long n)
{
  return n;
}

long long
ld_to_ll (long double n)
{
  return n;
}

main ()
{
  long long n;

  if (ll_to_ld (10LL) != 10.0)
    abort ();

  if (ld_to_ll (10.0) != 10)
    abort ();

  exit (0);
}
-----------------------^

 The first conversion gets through allright (long-long to long
double), but the second conversion crashes.  It is function
"ld_to_ll()" that goes wrong.  The RTL for the first two functions,
from before the flow-analysis (cse2) looks like this:
 (NB: first four parameters with a size <= 32bit are passed in
registers r10..r13; DImode is passed by reference, except for library
calls.  All scalar return values start at r10.  The type "long double" 
is SFmode)

-----------------------v
;; Function ll_to_ld

;; Processing block from 2 to 0, 14 sets.
(note 2 0 3 "" NOTE_INSN_DELETED)

(insn 3 2 4 (set (reg/v:SI 21)
        (reg:SI 10 r10)) 24 {movsi} (nil)
    (nil))

(insn 4 3 5 (clobber (reg/v:DI 22)) -1 (nil)
    (nil))

(insn 5 4 6 (set (subreg:SI (reg/v:DI 22) 0)
        (mem:SI (reg/v:SI 21))) 24 {movsi} (nil)
    (nil))

(insn 6 5 7 (set (reg:SI 23)
        (plus:SI (reg/v:SI 21)
            (const_int 4))) 50 {addsi3} (nil)
    (nil))

(insn 7 6 8 (set (subreg:SI (reg/v:DI 22) 1)
        (mem:SI (reg:SI 23))) 24 {movsi} (nil)
    (nil))

(note 8 7 9 "" NOTE_INSN_FUNCTION_BEG)

(note 9 8 11 "" NOTE_INSN_DELETED)

(note 11 9 12 "" NOTE_INSN_DELETED)

(insn 12 11 13 (set (reg:SI 25)
        (symbol_ref:SI ("__floatdisf"))) 24 {movsi} (nil)
    (expr_list:REG_EQUAL (symbol_ref:SI ("__floatdisf"))
        (nil)))

(insn 13 12 14 (clobber (reg:DI 10 r10)) -1 (nil)
    (insn_list:REG_LIBCALL 17 (nil)))

(insn 14 13 15 (set (reg:SI 10 r10)
        (subreg:SI (reg/v:DI 22) 0)) 24 {movsi} (nil)
    (nil))

(insn 15 14 16 (set (reg:SI 11 r11)
        (subreg:SI (reg/v:DI 22) 1)) 24 {movsi} (nil)
    (nil))

(call_insn/u 16 15 17 (parallel[ 
            (set (reg:SF 10 r10)
                (call (mem:QI (reg:SI 25))
                    (const_int 0)))
            (clobber (reg:SI 16 srp))
        ] ) 165 {call_value+1} (nil)
    (nil)
    (expr_list (use (reg:DI 10 r10))
        (nil)))

(insn 17 16 18 (set (reg:SF 24)
        (reg:SF 10 r10)) 31 {movsf} (nil)
    (insn_list:REG_RETVAL 13 (expr_list:REG_EQUAL (float:SF (reg/v:DI 22))
            (nil))))

(insn 18 17 19 (set (reg/i:SF 10 r10)
        (reg:SF 24)) 31 {movsf} (nil)
    (nil))

(insn 19 18 0 (use (reg/i:SF 10 r10)) -1 (nil)
    (nil))

;; Function ld_to_ll

;; Processing block from 2 to 0, 15 sets.
(note 2 0 3 "" NOTE_INSN_DELETED)

(insn 3 2 4 (set (reg/v:SF 21)
        (reg:SF 10 r10)) 31 {movsf} (nil)
    (nil))

(note 4 3 5 "" NOTE_INSN_FUNCTION_BEG)

(note 5 4 7 "" NOTE_INSN_DELETED)

(note 7 5 8 "" NOTE_INSN_DELETED)

(insn 8 7 9 (set (reg:SI 23)
        (symbol_ref:SI ("__fixsfdi"))) 24 {movsi} (nil)
    (expr_list:REG_EQUAL (symbol_ref:SI ("__fixsfdi"))
        (nil)))

(insn 9 8 10 (set (reg:SF 10 r10)
        (reg/v:SF 21)) 31 {movsf} (nil)
    (insn_list:REG_LIBCALL 13 (nil)))

(call_insn/u 10 9 11 (parallel[ 
            (set (reg:DI 10 r10)
                (call (mem:QI (reg:SI 23))
                    (const_int 0)))
            (clobber (reg:SI 16 srp))
        ] ) 165 {call_value+1} (nil)
    (nil)
    (expr_list (use (reg:SF 10 r10))
        (nil)))

(insn 11 10 12 (clobber (reg:DI 22)) -1 (nil)
    (nil))

(insn 12 11 13 (set (subreg:SI (reg:DI 22) 0)
        (reg:SI 10 r10)) 24 {movsi} (nil)
    (nil))

(insn 13 12 14 (set (subreg:SI (reg:DI 22) 1)
        (reg:SI 11 r11)) 24 {movsi} (nil)
    (insn_list:REG_RETVAL 9 (expr_list:REG_EQUAL (fix:DI (reg/v:SF 21))
            (nil))))

(insn 14 13 15 (clobber (reg:DI 22)) -1 (nil)
    (nil))

(insn 15 14 16 (set (subreg:SI (reg:DI 22) 0)
        (subreg:SI (reg:DI 22) 0)) 24 {movsi} (nil)
    (nil))

(insn 16 15 17 (set (subreg:SI (reg:DI 22) 1)
        (subreg:SI (reg:DI 22) 1)) 24 {movsi} (nil)
    (nil))

(insn 17 16 18 (clobber (reg/i:DI 10 r10)) -1 (nil)
    (nil))

(insn 18 17 19 (set (subreg:SI (reg/i:DI 10 r10) 0)
        (subreg:SI (reg:DI 22) 0)) 24 {movsi} (nil)
    (nil))

(insn 19 18 20 (set (subreg:SI (reg/i:DI 10 r10) 1)
        (subreg:SI (reg:DI 22) 1)) 24 {movsi} (nil)
    (nil))

(insn 20 19 0 (use (reg/i:DI 10 r10)) -1 (nil)
    (nil))

;; Function main

(the rest not included, to spare you from dizzyness)
-----------------------^

Look at insn #14 in ld_to_ll().  This is a "clobber", to mark that
pseudo-reg #22 is pining for the fjords.
 However, the next instruction, supposedly clobbering pseudo-reg #22, is
just a nop-move; (set r22 r22).
 This means it is actually USING pseudo-reg #22; not just smashing its
previous value.  The flow analysis sees the clobber, assumes that any
data pseudo-reg #22 depends on can be ignored, and the entire library
call gets optimized away, leaving just the instructions after the
"clobber".

 Now, who emitted that "clobber"?  Well, if we look at the end of the
function optabs.c:expand_fix(), we see a sequence that copies the
output of the library call (rtx "target") to where we really want it
(rtx "to"):

-----------------------v
  ...
  if (GET_MODE (to) == GET_MODE (target))
    emit_move_insn (to, target);
  else
    convert_move (to, target, 0);
}

  ...
-----------------------^

 If these two rtx:es are the same, ("target" may actually be an
intermediate register), this makes a NOP-move.  What is worse, the
function "emit_move_insn()" gets a mode larger than what has a machine
description.  This causes the companion function to
"emit_move_insn()"; "emit_move_insn_1()", to move the entity in parts:
(excerpt from "expr.c:emit_move_insn_1()"):

-----------------------v
  ...
  /* This will handle any multi-word mode that lacks a move_insn pattern.
     However, you will get better code if you define such patterns,
     even if they must turn into multiple assembler instructions.  */
  else if (GET_MODE_SIZE (mode) > UNITS_PER_WORD)
    {
      rtx last_insn = 0;
      rtx insns;
      
#ifdef PUSH_ROUNDING

      /* If X is a push on the stack, do the push now and replace
	 X with a reference to the stack pointer.  */
      if (push_operand (x, GET_MODE (x)))
	{
	  anti_adjust_stack (GEN_INT (GET_MODE_SIZE (GET_MODE (x))));
	  x = change_address (x, VOIDmode, stack_pointer_rtx);
	}
#endif
			     
      /* Show the output dies here.  */
      emit_insn (gen_rtx (CLOBBER, VOIDmode, x));
  ...
      (the rest of the code emits the move-by-parts)
-----------------------^

(Note: no PUSH_ROUNDING here, that part is not involved.)

This clearly has the intent to indicate that the output of the
move-by-parts sequence is dead.  Too bad it was the same as the input...


Now think of this:  Why didnt the same story happen to the output of
the library call in "ll_to_ld()".  It goes through a simular function;
"optabs.c:expand_float()".  That function looks very much the same in
structure, although it obviously does the opposite conversion.

But get this; at the end of the function "optabs.c:expand_float()", it
looks like this:

-----------------------v
  ...
  /* Copy result to requested destination
     if we have been computing in a temp location.  */

  if (target != to)
    {
      if (GET_MODE (target) == GET_MODE (to))
	emit_move_insn (to, target);
      else
	convert_move (to, target, 0);
    }
  ...
-----------------------^

So, that code actually checks if "to" is the same as "target", and
skips any nop-move!  If there is a good explanation why this is not
done in expand_fix() too, then at least it is not mentioned in any
nearby comment.  Ergo, it's a bug.

+++++++++++++++++++++++
Now for two ways to solve this:

The obvious fix is to do the same change to "optabs.c:expand_fix()" as
in "optabs:expand_float()" ("diff -cp" follows):

-----------------------v
*** optabs.c	Sat Oct 21 23:16:13 1995
--- /usr/tmp/optabs.c.FIXED	Sat Jan  6 15:30:29 1996
*************** expand_fix (to, from, unsignedp)
*** 3877,3886 ****
  				   GET_MODE (to), from));
      }
        
!   if (GET_MODE (to) == GET_MODE (target))
!     emit_move_insn (to, target);
!   else
!     convert_move (to, target, 0);
  }
  
  static optab
--- 3877,3895 ----
  				   GET_MODE (to), from));
      }
        
!   /* Do NOT copy, if "to" is the same as "target".  See
!      expand_float().  It used to cause a bogus "clobber" of the
!      input, causing it's ancestors to be deleted.  Hopefully the call
!      is just unnecessary now, causing extra time in optimization, or
!      some extra instructions.
!      / Hans-Peter.Nilsson@axis.se */ 
!   if (target != to)
!     {
!       if (GET_MODE (to) == GET_MODE (target))
!         emit_move_insn (to, target);
!       else
!         convert_move (to, target, 0);
!     }
  }
  
  static optab
-----------------------^

ChangeLog entry for this:
-----------------------v
Sat Jan  6 03:27:49 1996  Hans-Peter Nilsson  <Hans-Peter.Nilsson@axis.se>

        *  optabs.c (expand_fix): Skip final copy of "target" to "to" if
        they are the same.

-----------------------^

Ok.  That fix will do, at least for this problem, without possibly
breaking anything.

 Now, why did "expr.c:emit_move_insn_1()" output a clobber in a case
where it obviously shouldn't?
 Is is reasonable to assume that all wanna-be callers to
"expr.c:emit_move_insn()" (there are 218 in gcc-2.7.2, including
c++-code) check for an identity in the supposed operands before the
call? 
 If we look in "optabs.c:expand_binop()", around line 964, we can see
a line which actually emits a verbatim instruction to do this kind of
nop-copy:

-----------------------v
      if (i == GET_MODE_BITSIZE (mode) / BITS_PER_WORD)
	{
	  rtx temp = emit_move_insn (target, target);

	  REG_NOTES (temp) = gen_rtx (EXPR_LIST, REG_EQUAL,
-----------------------^

 I do not know whether the code in general should emit these
constructions.  Any conclusion would probably be rushed.  My humble
suggestion is to stop the "clobber" from being emitted when something
is copied onto itself ("diff -cp" follows: 

-----------------------v
*** expr.c	Sun Nov 26 17:18:07 1995
--- /usr/tmp/expr.c.FIXED	Sat Jan  6 15:30:41 1996
*************** emit_move_insn_1 (x, y)
*** 1987,1994 ****
  	}
  #endif
  			     
!       /* Show the output dies here.  */
!       emit_insn (gen_rtx (CLOBBER, VOIDmode, x));
  
        for (i = 0;
  	   i < (GET_MODE_SIZE (mode)  + (UNITS_PER_WORD - 1)) / UNITS_PER_WORD;
--- 1987,2001 ----
  	}
  #endif
  			     
!       /* Do not emit a clobber if source equals destination.  That
!          will cause any data-ancestors of the input to be removed in
!          optimization.
!          ??? Maybe check equality with some kind of rtx_equal_p() or
!          reg_overlap_mentioned_p()?
!          / Hans-Peter.Nilsson@axis.se */
!       if (x != y)
!         /* Show the output dies here.  */
!         emit_insn (gen_rtx (CLOBBER, VOIDmode, x));
  
        for (i = 0;
  	   i < (GET_MODE_SIZE (mode)  + (UNITS_PER_WORD - 1)) / UNITS_PER_WORD;
-----------------------^

ChangeLog entry:
-----------------------v
Sat Jan  6 03:30:23 1996  Hans-Peter Nilsson  <Hans-Peter.Nilsson@axis.se>

        * expr.c (emit_move_insn_1): Do not emit a "clobber" when moving
        a big mode by parts, and the source equals the destination.

-----------------------^

I have yet to verify that both changes work; I will use both changes
when going through our compiler test suite.
Anybody who asks for it, will be e-mailed the results in a "It works"
/ "It does not work, use this instead ..." type response.

 The fixes certainly checked out fine with the small example above.

Questions or requests for clarification are welcome.

/Thanks for your time.
-- 
Hans-Peter Nilsson, Axis Communications AB, S - 223 70 LUND, SWEDEN
Hans-Peter.Nilsson@axis.se | http://www.axis.se/ | Tel +46 46191867,191800
Fax +46 46136130 | "If this is paradise, I wish I had a lawnmower" / D. Byrne