Re: [Patch : H8300] Fix regressions for bit related instructions

[Jeff Law <law at redhat dot com>]

On 02/16/10 23:47, Naveen H. S wrote:
> Hi Jeff,
>
>    
> > > If there is a hunk that is directly responsible for fixing bugs, we
> > > can break that out into its own patch that could be installed now
> > >        
> Please find attached the "h8sx.patch" patch which fixes regressions
> for H8SX and related targets.
> The patch basically disallows use of register operands in bit patterns
> for H8SX target. H8SX code results in ICE when trying to generate
> SUBREG(gen_rtx_SUBREG) for bit patterns. Hence, bit patterns of H8SX
> was restricted to use only memory mode. As H8SX target is a higher
> version of H8300, it generates memory addressing RTL pattern for bit
> patterns. Hence this approach would optimize the code in H8SX as it
> reduces the overhead of loading registers before operating on bit
> patterns.
>
> We had already posted the initial patch at following link:-
> http://gcc.gnu.org/ml/gcc-patches/2008-10/msg00674.html
> However, we could not follow up on the patch due to some other
> priorities. Please let us know whether it is the correct approach to
> solve the ICE generated with H8SX target or let us where to modify the
> code to fix these errors.
>    
Ah.  I recall that patch.    My first reaction when it was posted in 
2008 was that it was papering over a bug elsewhere.  I didn't dive 
further into it because I expected you to.   I still feel that initial 
review is correct.

I did build a cross compiler so that I could take a closer look and see 
if there was anything obviously wrong which would ultimately lead to the 
assertion failure in validate_subreg.    I think the fundamental problem 
here is someone didn't heed the comment at the start of h8300.md when SX 
support was added and I probably missed that buglet when the SX code was 
reviewed.

;; There's currently no way to have an insv/extzv expander for the H8/300H
;; because word_mode is different for the H8/300 and H8/300H.

That comment should include the H8/S and H8/SX everywhere H8/300H is 
mentioned.

If we look at the expanders we have:

(define_expand "insv"
  [(set (zero_extract:HI (match_operand:HI 0 "general_operand" "")
                         (match_operand:HI 1 "general_operand" "")
                         (match_operand:HI 2 "general_operand" ""))
        (match_operand:HI 3 "general_operand" ""))]
  "TARGET_H8300 || TARGET_H8300SX"


(define_expand "extzv"
  [(set (match_operand:HI 0 "register_operand" "")
        (zero_extract:HI (match_operand:HI 1 "bit_operand" "")
                         (match_operand:HI 2 "general_operand" "")
                         (match_operand:HI 3 "general_operand" "")))]
  "TARGET_H8300 || TARGET_H8300SX"




The fundamental problem is the various operands on the insv/extv 
patterns specify HImode.  I believe at the time I wrote that comment 
(1995?) the backend was required to specify modes for the insv/extv 
operands in the expander -- omitting the modes in the pattern was not 
supported.   Thus there was no way to properly  support insv/extv on the 
H8/300H, H8/S or H8/SX targets.

The assertion failures you're seeing occur because we have an SImode 
operand, but the extzv pattern requires a HImode operand.  We try to 
create a suitable SUBREG to get at the low word via this code:

      /* If op0 is a register, we need it in EXT_MODE to make it
         acceptable to the format of ext(z)v.  */
      if (REG_P (xop0) && GET_MODE (xop0) != ext_mode)
        xop0 = gen_rtx_SUBREG (ext_mode, xop0, 0);

Where ext_mode will be HImode and xop0 will be an SImode object.

This isn't going to work on a big endian target.  That code should 
probably be
 xop0 = gen_lowpart_SUBREG (ext_mode, xop0);

Similarly for the code in store_bit_field_1.

I *think* that's enough to fix this testcase and I think it's correct 
for other targets as well.  I'm playing around with that change right now.

Long term, the right thing to do is remove the modes on the operands of 
the extv/insv expanders and change the expander's condition to allow it 
to be used on all the H8 targets (insv/extv operands are assumed to be 
WORD_MODE if they are not explicitly specified).  This may require 
twiddling matching patterns or creating new patterns.

> > > If you could also verify that a copyright assignment is in place for:
> > >        
> KPIT has signed copyright assignment and some of our patches are already
> accepted and present in current GCC sources. Please let us know whether
> we should provide the signed copyright assignment approval mail.
>    
Thanks.  I figured that was the case, I just wanted to be sure.

jeff