egcs-980122: problem with recog.c

[Alasdair Baird]

I have found what appears to be a problem with recog.c in
compiling some of the X11 sources on an i386 platform with
optimization on.  The result of this bug was to generate
the code "xorl %ecx,%dx", code which assemblers tend not to
find amusing.

In looking through the output of the various passes of the
compiler I narrowed the problem down to be occuring in the
regmove pass.  What was happening was that the function
optimize_reg_copy_3 was being called to move a zero_extend
backward in the RTL; the relevant pre-regmove RTL was:


(insn 31 27 33 (set (reg/v:QI 29)
        (mem/s:QI (plus:SI (reg:SI 27)
                (reg:SI 24)))) 63 {movqi+1} (insn_list 27 (nil))
    (expr_list:REG_DEAD (reg:SI 27)
        (expr_list:REG_DEAD (reg:SI 24)
            (nil))))

   <irrelevant instructions elided>

(insn/i 37 36 38 (set (reg/v:QI 33)
        (xor:QI (mem:QI (const:SI (plus:SI (symbol_ref:SI ("r"))
                        (const_int 1))))
            (reg/v:QI 29))) 191 {xorqi3} (insn_list 31 (nil))
    (nil))

(insn/i 38 37 39 (set (reg:SI 34)
        (zero_extend:SI (reg/v:QI 29))) 93 {zero_extendqisi2} (nil)
    (expr_list:REG_DEAD (reg/v:QI 29)
        (nil)))


What optimize_reg_copy_3 does is to change the mode of (reg/v:QI 29)
to (reg/v:SI 29) in the middle section, having first replaced the
(zero_extend:SI (reg/v:QI 29)) at the end with (reg/v:SI 29) and
having put the zero extend in with the initial load of the register.

Of course this isn't quite right; all the intervening occurances of
(reg/v:SI 29) between the initial load and the register dying were
actually supposed to be made as QIs, so next optimize_reg_copy_3
goes thought that intervening RTL putting in the correct subreg
wording, i.e. (subreg:QI (reg/v:SI 29) 0) instead (reg/v:SI 29).

All fine and dandy, if a bit cumbersome.

The problem comes in the function validate_replace_rtx_1 within recog.c
which is actually used to do the final bit of substitution.  This function
is recursive (bet you can see where this is leading...) and operates on
the central part of the RTL which by now looks something like:

(insn/i 37 36 38 (set (reg/v:QI 33)
        (xor:QI (mem:QI (const:SI (plus:SI (symbol_ref:SI ("r"))
                        (const_int 1))))
            (reg/v:SI 29))) -1 


It carries out its work by considering what sort of RTX is within this and
then recursively calling itself on its constituent RTXs.

The first time through it hits its fall-through; there is no special
handling for SETs so it calls itself first to do the substitution on
the XOR source, then on the REG destination.

Within the recursion for the XOR it first hits some code that deals
specially with commutative and comparison operations before trundling
on.  For commutative and comparison operations such as XOR it first
performs the substitution on both RTXs of the operator; this does have
the desired effect of changing (reg/v:SI 29) to (subreg:QI (reg/v:SI 29) 0).

After dealing specially with commutative and comparison operations
there is a chance for some special processing on different expression
codes, e.g. PLUS and MINUS and some others.  In this case, XOR, no
further processing happens in this section.

However after doing no special processing it will still execute the
fall-through path, which performs the substitution on the constituent
parts of the XOR.  But this has already been done, and making the
substitution of (subreg:QI (reg/v:SI 29) 0) for (reg/v:SI 29) in
(subreg:QI (reg/v:SI 29) 0) isn't going to win prizes for being a smart
thing to do.

The result of this is the RTL ends up apparently unsubstituted (!), hence
the funny "xorl %ecx,%dx"; why the whole thing doesn't melt down I haven't
looked into.

However, by preventing the recursion happening a second time---making it
conditional on the expression code being neither a commutative nor a
comparison operator---the substitution happens properly and the nasty
"xorl %ecx,%dx" goes away.  Oh, and the compiler still bootstraps.


   Alasdair.



Mon Jan 26 00:34:56 1998  Alasdair Baird <alasdair@wildcat.demon.co.uk>

        * recog.c (validate_replace_rtx_1): Ensure substitution to 
        the arguments of commutative and comparison operators happens
        only once.



*** recog.c.ORIG	Wed Jan 14 23:10:30 1998
--- recog.c	Sun Jan 25 23:36:29 1998
***************
*** 510,523 ****
        break;
      }
        
!   fmt = GET_RTX_FORMAT (code);
!   for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
      {
!       if (fmt[i] == 'e')
! 	validate_replace_rtx_1 (&XEXP (x, i), from, to, object);
!       else if (fmt[i] == 'E')
! 	for (j = XVECLEN (x, i) - 1; j >= 0; j--)
! 	  validate_replace_rtx_1 (&XVECEXP (x, i, j), from, to, object);
      }
  }
  
--- 510,526 ----
        break;
      }
        
!   if (GET_RTX_CLASS (code) != '<' && GET_RTX_CLASS (code) != 'c')
      {
!       fmt = GET_RTX_FORMAT (code);
!       for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
!         {
!           if (fmt[i] == 'e')
!             validate_replace_rtx_1 (&XEXP (x, i), from, to, object);
!           else if (fmt[i] == 'E')
!             for (j = XVECLEN (x, i) - 1; j >= 0; j--)
!               validate_replace_rtx_1 (&XVECEXP (x, i, j), from, to, object);
!         }
      }
  }