Re: [PATCH][2/3] Instruction patterns changes and instruction attributes

[Roman Zippel <zippel at linux-m68k dot org>]

Hi,

On Wed, 29 Aug 2007, Maxim Kuvyrkov wrote:

> > > 1. Splits.
> > > m68k backend is the eldest in gcc.  It employs several techniques that
> > > are now considered obsolete, one of such things, that makes scheduling
> > > impossible, is using C code to emit best asm statement for a given rtl
> > > insn.  This patch converts the most frequently used (on CFv2) patterns
> > > to use constraints and alternatives instead of falling back to C.
> > 
> > m68k should really use even more splits, e.g. output_move_double in its
> > current form should go away completely. Most of these moves should already
> > splitted before reload, reload might generate a few more, but these are
> > relatively simple to split later, so that at the point of assembly output
> > none of them are left. IMO this change could be done independently of the
> > attribute changes.
> 
> Yes, m68k backend can surely benefit from more splits, and I plan to add more
> of them when I have time.  It's not an easy task though to preserve all the
> alternatives that are coded in C.

The point is that you add the scheduling support without any of the IMO 
necessary cleanups, thus increasing the complexity of the m68k port and 
only making later cleanups more difficult. E.g. overloading 
output_move_double is a bad idea in this regard and you should rather add 
a new more sane emit function. I'm sure a few more cleanups beforehand 
would avoid this rather big patch.

> > > 2. Attributes.
> > > In order to facilitate pipeline model with information about
> > > instructions, several instruction attributes were added.  Due to
> > > genattrtab limitations not all attributes are written in lisp-like
> > > language, but rather in C.  Attributes can be optimized later, when the
> > > whole set of them is stable (that is after adding scheduling support for
> > > other processors).
> > 
> > Could you please explain the op_type attributes a little? It looks a little
> > more complex then necessary to me. In general m68k instructions are rather
> > simple in this regard. Most instructions have only a single memory operand,
> > which is either a read-only or read-write operand. The only really relevant
> > exception is the move instruction, which can have two memory operands, which
> > are read-only or write-only. (There are other exceptions like "subx.l
> > -(%a0),-(%a1)", but I couldn't find any which gcc really uses right now.)
> > This means in many cases you duplicate information, which is already part of
> > the instruction pattern.
> 
> opx_type, opy_type, op_mem attributes represent information about instruction
> pattern in a way DFA is most comfortable with.  These attributes do not carry
> any additional information regarding instruction pattern.

Well, that's sort of the problem :), it mostly doesn't contain 
_additional_ information, it mostly duplicates information.
E.g. the opx/opy attributes can also be determined by looking at the RTL 
pattern, the same goes often for the type attribute, but here concerns me 
even more that you encoded the size into it.
Where is op_mem actually used? My best guess is cf-v2.md, which is missing 
in the 3rd patch. In general op_mem looks somewhat complex, it's mixing 
access pattern from move and arithmetic instructions, but I had to see how 
it's actually used.

> In general I'd prefer to only
> > annotate the exceptions and I'd try as much possible by analyzing the
> > instruction patterns first (this way the whole annotation also hasn't to be
> > repeated for other cpu's).
> 
> This is precisely the way its done.

I disagree, you barely extract information from the instruction pattern 
beyond the operands.

> > I think that names like OP_TYPE_MEM1 are not a good a choice, after a month
> > I had to look these up again to know what they mean.
> 
> Numbered classification of addressing is used throughout m68k backend and its
> documentation.

Where? The documentation usually provides a descriptive name as well. 
In the back end we want to get away from using magic numbers.

>  What do you think would be a better choice for memory
> addressing modes?

For example: MEM_INDIRECT, MEM_DISP16, MEM_POSTINC, MEM_PREDEC, 
MEM_IDX_BRIEF, MEM_IDX_FULL, MEM_ABS16, MEM_ABS32

>  The basic mem
> > type classification should be done by m68k_decompose_address, as it's useful
> > for other things too (e.g. instruction cost), 
> 
> This is the way its done.

No, you're doing it in sched_address_type(). struct m68k_address should 
already have an enumerated type describing the address type.

> but this needs a
> > little more detail (e.g. brief or full extension word for mode 6).
> 
> At this point that detail would be unused.  It should be added when we need
> it.

You don't need it, because you're focused on ColdFire.

> > What are your long term plans with the split attribute? IMO at some point it
> > should become superfluous, but then we may have to remove a lot of "done"
> > attributes. Here I'd also think it would be better to only annotate the
> > exceptions, it might be possible to do a basic classifiction already be
> > looking at the output pattern, e.g. if calls a C function or uses ";" it may
> > need more splitting.
> 
> Split attribute will be removed.  I introduced it to track statistics and
> prioritize instruction patterns that should be converted from calling C
> functions to use constraints and alternatives.

You didn't comment on my suggestion. It would also be nice to make the 
split statistics independent of the scheduling.

bye, Roman