Re: Reload bug

[Jan Hubicka <jh at suse dot cz>]

> > I am attaching full one.  It fixes the 3.2 testcases I have...
> > Does it appear to make sense to you?
> 
> The locations from which you call the new subreg_offset_representable_p() 
> look sensible. But I think there are some duplicate logic in 
> reload_inner_reg_of_subreg() now.
Yes, I would like to remove it separately so in the case of problem we
will be able to determine what part has actually caused that.
> 
> + /* This function returns true when the offset is representable via
> +    subreg_offset in the given regno.
> 
> The function appears to assume that reg XREGNO supports mode YMODE. Is that 
> by design? If so, I think you should mention it in the header comment.

Hmm, I am not certain what should happen there.  We can either verify it
here or the place we are doing so already...
> +   nregs_xmode = HARD_REGNO_NREGS (xregno, xmode);
> +   nregs_ymode = HARD_REGNO_NREGS (xregno, ymode);
> + 
> +   /* paradoxical subregs are always valid.  */
> +   if (offset == 0
> +       && nregs_ymode > nregs_xmode
> +       && (GET_MODE_SIZE (ymode) > UNITS_PER_WORD
> + 	  ? WORDS_BIG_ENDIAN : BYTES_BIG_ENDIAN))
> +     return true;
> + 
> +   /* Lowpart subregs are always valid.  */
> +   if (offset == subreg_lowpart_offset (ymode, xmode))
> +     return true;
> 
> I think you should mention that the second "if" catches paradoxical subregs 
> too.

Hmm, you are right. Then we can remove the first if too :)
> 
> + #ifdef ENABLE_CHECKING
> +   /* This should always pass, otherwise we don't know how to verify the
> +      constraint. 
> + 
> +      These conditions may be relaxed but subreg_offset would need to be
> +      redesigned.  */
> +   if (GET_MODE_SIZE (xmode) % GET_MODE_SIZE (ymode)
> +       || GET_MODE_SIZE (ymode) % nregs_ymode
> +       || mode_for_size (GET_MODE_SIZE (ymode) / nregs_ymode,
> + 	      		MODE_INT, 0) == VOIDmode
> +       || nregs_xmode % nregs_ymode)
> +     abort ();
> + #endif
> + 
> +   /* The XMODE value can be seen as an vector of NREGS_XMODE
> +      values.  The subreg must represent an lowpart of given field.
> +      Compute what field it is.  */
> +   offset -= subreg_lowpart_offset (mode_for_size (GET_MODE_SIZE (ymode)
> + 			  			  / nregs_ymode,
> + 						  MODE_INT, 0), xmode);
> 
> I'm clueless here: I think I understand what you want to do but I have a 
> mixed feeling about the formula. In particular,
> 
> 	GET_MODE_SIZE (ymode) / nregs_ymode
> 
> looks weird for a size. Didn't you forget to re-multiply by nregs_ymode?

No, I am actually trying to compute the size of the one ymode register
in order to be able to compensate the lowpart operation.
Imagine  (subreg:QI (reg:DI) 3) is valid on little endian with
nregs_ymode == 2
> 
> +   /* size of ymode must not be greater than the size of xmode.  */
> +   mode_multiple = GET_MODE_SIZE (xmode) / GET_MODE_SIZE (ymode);
> +   if (mode_multiple == 0)
> +     abort ();
> 
> All paradoxical subregs should already have been caught at that point, no?

Yes, it is just pasto.

Honza
> 
> +   y_offset = offset / GET_MODE_SIZE (ymode);
> +   nregs_multiple =  nregs_xmode / nregs_ymode;
> + #ifdef ENABLE_CHECKING
> +   if (offset % GET_MODE_SIZE (ymode)
> +       || mode_multiple % nregs_multiple)
> +     abort ();
> + #endif
> +   return (!(y_offset % (mode_multiple / nregs_multiple)));
> + }
> 
> I think you're right about the condition.
> 
> -- 
> Eric Botcazou