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

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

Hi,

On Mon, 3 Sep 2007, Maxim Kuvyrkov wrote:

> > 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.
> 
> I do not agree with you here.  Scheduling support is rather independent part
> of the target description.  It gets all information it needs from the
> instruction attributes.

So why don't you split the patch further?

> > E.g. overloading output_move_double is a bad idea in this regard and you
> > should rather add a new more sane emit function. 
> 
> At first I did write a separate function that emitted RTL for move_double.
> Later on, when it was finished, I discovered that it looks much like
> output_move_double () and that I was simply duplicating its logic in RTL
> terms.  At some point (when there'll be no references to output_move_double)
> overloading should go away leaving just the RTL version.  I would appreciate
> any help in making that moment closer.

emit_move_double() should actually look quite different - especially a 
lot simpler, a number of move combination should be avoided in first 
place, any register adjustments should be avoided.

> > I'm sure a few more cleanups beforehand would avoid this rather big patch.
> 
> Probably.  But on the other hand, it is always better to have something on
> hand that would benefit from the cleanup.  Changes for the sake of changes are
> not as appreciated as changes that make difference :)

It makes a patch a lot harder to review.

> Ok, a little bit of statistics:
> 
> - opx attribute is explicitly defined for 3 patterns.  X operand by default is
> operand[0], but for those 3 patterns it is operand[1].
> - opy attribute is explicitly defined for 11 patterns.  Y operand by default
> is operand[1], but for those patterns it is operand[2] - that is because the
> patterns alias 0 and 1 operands (e.g. addsi3, subsi3)
> - opy_type attribute is explicitly defined for 4 patterns only.  For every
> other pattern it is inferred from analyzing the Y operand of the instruction.
> - opx_type attribute is always inferred from the X operand.
> 
> As none of the information that is explicitly specified can be easily obtained
> by analyzing instruction pattern I don't see duplication here.
> 
> > the same goes often for the type attribute, but here concerns me even more
> > that you encoded the size into it.

Most of the problem is propably, that your 3rd patch is incomplete. My 
suspicion is that you barely take advantage of the m68k instruction 
format. It should be possible to derive some of these from the type.

> Can you please explain what do you mean by 'encoded the size into it'?

E.g. clr_l, clr_w, clr_b.

> The scheduling description I've submitted follows the best practices accepted
> in gcc:

Well, m68k has not exactly the man power of other ports and due to its age 
it needs more work to really support instruction scheduling, that's why I 
think we should try to keep it as simple as possible, which includes to 
do some cleanups first.

> > I disagree, you barely extract information from the instruction pattern
> > beyond the operands.
> 
> Analyzing instruction patterns is a bad thing because it tends to break upon
> adding any new patterns.  Operands, on the other hand are much more stable and
> can be used to extract information for scheduling.

IMO the worst thing that happens is that a new pattern isn't recognized, 
but in your case the wrong operand may be taken and there is pretty much 
no way to detect this.

> > No, you're doing it in sched_address_type(). struct m68k_address should
> > already have an enumerated type describing the address type.
> 
> Well, it doesn't have that field.
> 
> > 
> > > 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.
> 
> Or one can say, because I don't have time for everything.  I think community
> would appreciate any cleanups to m68k backend you've mentioned.

I don't have much time either and I'd like to spend it on the other 
things.

> > > 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.
> 
> Do you mean 'to only annotate the exceptions' or 'do a basic classification
> already by looking at the output pattern'?  As this attribute will be removed
> and serves a debugging role only, I don't think it worth that much attention.
> Anyway, on the first one: I agree that only exceptions should be annotated and
> please point out where I've done opposite so that I'll can fix it.  And on the
> second one: It seems too hackish for me.

Well, since it's only a debugging feature, what's the problem, if it's a 
little hackish?
IMO this should really be separate feature, as it's useful beyond 
scheduling.

Some other things: What's up with the /*|*/ comments?
I also don't think it's a good idea to document all constraints, some 
should be left to internal use. It also seems that it's already incorrect 
(e.g. Cmvq).

bye, Roman