Re: [PATCH] Add new target h8300-*-linux

[Jeff Law <law at redhat dot com>]

On 04/19/2015 10:51 PM, Yoshinori Sato wrote:
> > > +  if (TARGET_H8300H && (TARGET_H8300S || TARGET_H8300SX))
> > > +    {
> > > +      target_flags ^= MASK_H8300H;
> > > +    }
> > I'm a bit concerned by this.  Why did you need to make this change?
>
> The flag is exclusion, but it's set both.
Hmmm, IIRC the port has many places where it may assume that H8300H is 
set for H8300S/H8300SX.  I did a very quick audit and saw:

I would recommend reviewing the extzv_16_8 pattern which has the 
condition "TARGET_H8300H" and changing the condition to
"TARGET_H8300H || TARGET_H8300S"  since AFAICT that pattern should work 
on both processor variants.

Similarly there's two peephole patterns have have conditions that looks 
like this:

 "(TARGET_H8300H || TARGET_H8300S)
    && peep2_reg_dead_p (1, operands[0])
    && ((TARGET_H8300H && INTVAL (operands[1]) == 3)
         || INTVAL (operands[1]) == 7
         || INTVAL (operands[1]) == 15
         || INTVAL (operands[1]) == 31
         || INTVAL (operands[1]) == 63
         || INTVAL (operands[1]) == 127
         || INTVAL (operands[1]) == 255)"


I'm pretty sure the second TARGET_H8300H should be (TARGET_H8300H || 
TARGET_H8300S).

In h8300.c::get_shift_alg, case HIshift, count 14, does this need to change?

            else if (TARGET_H8300H)
                {
                  info->special = 
"shll.b\t%t0\n\tsubx.b\t%s0,%s0\n\tshll.b\t%t0\n\trotxl.b\t%s0\n\texts.w\t%T0";
                  info->cc_special = CC_SET_ZNV;
                }
              else /* TARGET_H8300S */
                gcc_unreachable ();

Similarly SImode shifts by 28-30 bits should be reviewed in a similar 
manner.  As should the implementation of h8300_shift_needs_scratch_p.

output_a_rotate also needs to be reviewed if you want to make the change 
to turn off H8300H when H8/S is true.  Similarly for 
compute_a_rotate_length.


There may be others, these are what I found with a very quick search. 
If there's not a compelling reason to make the change, I'd recommend 
against it.



Jeff