Re: [PATCH] AVR: rev.2: implement HI/SI logic operations and sign/zero extension by define_insn_and_split instead of define_insn

[Björn Haase <bjoern dot m dot haase at web dot de>]

Denis Chertykov wrote
> > >  	* config/avr/avr.md:
> > >  	replace magic numbers for unspec numbering by (define_constants ).
> > >  	add new unspec for generation of sign extension byte.
> > >  	extend define_insn to define_insn_and_split for the patterns...
> > >  	"andhi" (here also tiny change in define_insn pattern), "andsi"
> > >  	"iorhi", "iorhi_clobber", "iorsi", "iorsi_clobber"
> > >  	"extendqihi2", "extendqisi2", "extendhisi2"
> > >  	"zero_extendqihi2", "zero_extendqisi2", "zero_extendhisi2"
> > >  	replace "xorhi" and "xorsi" patterns by expanders
> >
> > Wrong change log entry.
>
> [...]
>
> How about right chahge log entry ?

Hi Denis,

There is a problem with my patch in its present form that has shown up the day 
before yesterday after I have been adapting the testcases in the 
gcc.c-torture/execute library for AVR. The problem shows up in concjunction 
with logic operations on bitfields. I will be sending you a very similar 
patch, then, also,  with a (hopefully) correct change log.

The presently existing problem with my patch has to do with that it generates 
at many places subreg expressions like

(set (subreg:QI (match dup 0) 0) (ior:QI (subreg:QI (match_dup 1) 0) 
(subreg:QI (match_dup 2) 0) ))

where (match_dup 2) is, e.g. (match_operand:HI 2 "register_operand" "r").
At first sight everything seems fine and since one assumes that (match_dup 2) 
is always a register. It has also worked for all the test cases in the 
testsuite, I have looked at so far. However, in rare occasions (e.g. when 
operating on long long bitfields in structs) (match_dup 2) could itself be a 
subreg:HI (reg:SI xx)  so that what one could get after the splitting is a 
nested subreg expression like

(set (subreg:QI (match dup 0) 0) (ior:QI (subreg:QI (match_dup 1) 0) 
(subreg:QI (subreg:HI (reg:SI 20) 0) 0) ))

. Gcc does not know how to handle these things. So instead of using a template 
like the one above that uses (subreg:QI (match_dup 2) 0), it seems that one 
is forced to check when splitting or when expanding RTL that operands[2] 
itself is not a subreg, or, respectively, one would need to write a handler 
that is sufficiently intelligent to "unnest" nested subreg expressions before 
generating the RTL.

The direct candidates for such helper functions are, of course gen_lowpart and 
gen_highpart. Unfortunately, these functions have the same bug as the 
explicit subreg pattern that I have been using: They will presently fail, as 
soon as they get a subreg as input operand.

So what I am presently working at is, trying to extend the functionality of 
gen_lowpart and gen_highpart so that they also can unnest nested subreg 
expressions. After the change the pattern above will probably read

(set (subreg:QI (match dup 0) 0) (ior:QI (subreg:QI (match_dup 1) 0)  
(match_dup 3) ))

with 

operands[3] = gen_lowpart (operands[2]);

instead of

(set (subreg:QI (match dup 0) 0) (ior:QI (subreg:QI (match_dup 1) 0) 
(subreg:QI (match_dup 2) 0) ))
.

. Apart of that probably everything would remain the same in the patch. 

In case that you have other general remarks, hints concerning coding style, 
etc. I'd appreciate your opinion. I know that I still have to learn quite a 
lot :-).

Yours,

Björn