Re: Instruction bundling on IA64 and the Intel Assembler

Re: Instruction bundling on IA64 and the Intel Assembler

[Richard Kenner]

    Unfortunately I have no time for this.

This is a perfect example of why patches should not be approved until
they are adequately documented.  It's *very* hard (often impossible)
to get that done later.  Indeed it's usually recommended to write the
comments *before* the code.

    As I wrote in my previous email I'll try to write more comments and
    divide the function on several ones when the main line is in stage 1.

Please write the comments now.  Dividing the function up can probably
safely be done now, but can indeed wait for stage 1.

Take a look at reorg.c.  That's code of roughly equivalent complexity.
Try to match the commenting style in that code.

Re: Instruction bundling on IA64 and the Intel Assembler

[Vladimir Makarov]

Richard Kenner wrote:
> 
>     Unfortunately I have no time for this.
> 

  I don't like when the sentence used without context.  I meant the
article here not the comments.  By the way when I wrote the patch, in my
opinion I wrote the comments enough to understand the code and e.g.
Richard Henderson understood the code.  It is far from the worst
example.  You can not understand the code without some work on the
code.  There should be a balance with comment size and the code. 
Sometimes a lot of comments is even worse.  I am going to write more
comments because you are asking me not because I am agree that the
comments are not enough.

> This is a perfect example of why patches should not be approved until
> they are adequately documented.  It's *very* hard (often impossible)
> to get that done later.  Indeed it's usually recommended to write the
> comments *before* the code.

Re: Instruction bundling on IA64 and the Intel Assembler

[Robert Dewar]

Richard Kenner wrote:

> A good guide (which few of us can meet, admittedly) is that you should
> be able to understand the most important part of the function just from
> the comments and with all the code deleted.

That's certainly the rule we try to follow. I looked at this particular
piece of code (bundling), and I must say I agree with Richard that it 
would be very helpful to have some more high level comments that 
describe what the code is doing at a higher level of abstraction than
the code itself. I quite understand that Vladimir doesn't need more
comments for himself, but to me if someone reads my code and notes that
it would be helpful to have more comments, that's a good enough reason
to provide those comments!

Re: Instruction bundling on IA64 and the Intel Assembler

[Richard Kenner]

    I don't like when the sentence used without context.  I meant the
    article here not the comments.  

I'm sorry.  It sounded like you were saying "to describe this would be
like writing an article and I don't have time to do that".

    By the way when I wrote the patch, in my opinion I wrote the comments
    enough to understand the code and e.g.  Richard Henderson understood
    the code. 

The issue isn't "is it possible to understand this code without comments".
Given enough work, it's possible to understand *machine code* without
comments. 

The purpose of comments is to make it *easier* to understand the code
so that needed changes can be made.  The present situation is a good
example of that.  To add code to mark the end of a bundle is something
that should be relatively easy.  It shouldn't be necessary to understand
every detail of the code in order to do that.  Instead, the comments
are supposed to explain the overview of each piece of code so somebody doesn't
have to spend the time to understand every piece of code in detail to
make a change like that.

    You can not understand the code without some work on the code.  

I don't follow.

    There should be a balance with comment size and the code.  

Sure.  If you have straightforward sequential code you can often get
away with few or no comments.  But a comment for every loop and for
every non-trivial "if" statement is an absolute minimum.

A good guide (which few of us can meet, admittedly) is that you should
be able to understand the most important part of the function just from
the comments and with all the code deleted.

    Sometimes a lot of comments is even worse.

Sure, but the code in question is *nowhere* near that level.

Re: Instruction bundling on IA64 and the Intel Assembler

[Vladimir Makarov]

Richard Kenner wrote:
> 
>     Could you be more specific.  I tried to describe the code in details.
>     I see the following comment for the function.
> 
> For one thing, the only description of the algorithm is "based on dynamic
> programming".  In what *way* is it based on it?  Somewhere in the
> function (but probably not in the front, since that describes the
> *external* view of the function, it would be good to give some overview
> of the algorithm.

  I could write an article about this.  This algorithm is a result of
several months of experiments and trying different approaches in insn
bundling.  Unfortunately I have no time for this.

  As I wrote in my previous email I'll try to write more comments and
divide the function on several ones when the main line is in stage 1.

Vlad

Re: Instruction bundling on IA64 and the Intel Assembler

[Richard Kenner]

    Could you be more specific.  I tried to describe the code in details.
    I see the following comment for the function.

For one thing, the only description of the algorithm is "based on dynamic
programming".  In what *way* is it based on it?  Somewhere in the
function (but probably not in the front, since that describes the
*external* view of the function, it would be good to give some overview
of the algorithm.

    /* First (forward) pass -- generates states. */
    /* Finding state with a minimal cost:  */
    /* Second (backward) pass: adding nops and templates:  */
    /* Insert additional cycles for MM-insns: */

These are more "tags" than comments.  Each comment should be at least a
full sentence and perhaps even a paragraph.

Let's just look at the first comment ("First (forward) pass --
generates states."  This has a whole bunch of sequential code, then one
short loop followed by a long loop which has at least two inner loops.
There are no comments saying what any of those are doing.  It's worse
later on.

There are also variables like "pos", where there is no idea what that is.
Also, you set INSN_MODE to TImode with no indication whatsoever of what
that flag is supposed to mean.

    IMHO this code is far from the worst one with the point view of comments.

True, and I complain about all of the others as well!

Re: Instruction bundling on IA64 and the Intel Assembler

[Vladimir Makarov]

Geert Bosch wrote:
> 
> However, my request still stands for more comments in this function.
> Vladimir, could you take care of that?
> 
> Also, it seems that it might help to break the function up in three
> smaller functions:
>    - the forward dynamic programming pass generating states
>      and finding states with minimal cost (about 150 lines)
>    - the backward pass adding nops and templates (about 125 lines)
>    - the Itanium-1 specific stuff (about 75 lines)
> 

Ok, I'll make it when the main line will be in stage 1.

Vlad

Re: Instruction bundling on IA64 and the Intel Assembler

[Geert Bosch]

On Dec 15, 2003, at 16:54, Zack Weinberg wrote:

> Hmm, you are probably right.  Well, how about this?  Always print
> "{.whatever" in the bundle_selector output template; then, whenever
> bundling() goes to emit a bundle_selector, have it also look three
> insns ahead and emit an "}" insn.

That sounds doable and would indeed be a good solution.
I'll investigate this route more.

> I think this will always be right, not interfere with labels and
>  directives, and shouldn't require you to comprehend this problem
>  function.

Yes, that would be a definite plus!
However, my request still stands for more comments in this function.
Vladimir, could you take care of that?

Also, it seems that it might help to break the function up in three
smaller functions:
   - the forward dynamic programming pass generating states
     and finding states with minimal cost (about 150 lines)
   - the backward pass adding nops and templates (about 125 lines)
   - the Itanium-1 specific stuff (about 75 lines)

   -Geert

Re: Instruction bundling on IA64 and the Intel Assembler

[Richard Kenner]

    NOPs have to be considered when determining bundle boundaries, so I think
    that the naive three-insns-farther approach will be correct.

Sure, but that's not what I meant.  What I meant is that you can't simply
look three insns ahead when the bundle start insn is emitted because the
nops haven't yet been emitted.  So you have to know where to look forward
and that requires *some* knowlege of the function in question.

Re: Instruction bundling on IA64 and the Intel Assembler

[Vladimir Makarov]

Richard Kenner wrote:
> 
> However, that still doesn't address part of Geert's comment, which is that
> there is little or no documentation in that function and certainly no
> overview comments saying what's going on.  Even if there is some simple
> way to deal with this issue, that documentation still needs to be provided.

  Could you be more specific.  I tried to describe the code in
details.  I see the following comment for the function.

/* The following function does insn bundling.  Bundling algorithm is
   based on dynamic programming.  It tries to insert different number of
   nop insns before/after the real insns.  At the end of EBB, it chooses
the
   best alternative and then, moving back in EBB, inserts templates for
   the best alternative.  The algorithm is directed by information
   (changes of simulated processor cycle) created by the 2nd insn
   scheduling.  */

  All loops have comments to (as Geert asked) like

/* First (forward) pass -- generates states. */
/* Finding state with a minimal cost:  */
/* Second (backward) pass: adding nops and templates:  */
/* Insert additional cycles for MM-insns: */

  The bundle state has many comments too

/* The following describes state of insn bundling.  */
struct bundle_state
{
  /* Unique bundle state number to identify them in the debugging
     output  */
  int unique_num;
  rtx insn;     /* corresponding insn, NULL for the 1st and the last
state  */
  /* number nops before and after the insn  */
  short before_nops_num, after_nops_num;
  int insn_num; /* insn number (0 - for initial state, 1 - for the 1st
                   insn */
  int cost;     /* cost of the state in cycles */
  int accumulated_insns_num; /* number of all previous insns including
                                nops.  L is considered as 2 insns */
  int branch_deviation; /* deviation of previous branches from 3rd
slots  */
  struct bundle_state *next;  /* next state with the same insn_num  */
  struct bundle_state *originator; /* originator (previous insn state) 
*/
  /* All bundle states are in the following chain.  */
  struct bundle_state *allocated_states_chain;
  /* The DFA State after issuing the insn and the nops.  */
  state_t dfa_state;
};


  I understand that the code is not trivial.  As many parts of gcc
some work is needed to understand code.  IMHO this code is far from
the worst one with the point view of comments.

Vlad

Re: Instruction bundling on IA64 and the Intel Assembler

[Geert Bosch]

On Dec 15, 2003, at 16:39, Zack Weinberg wrote:
> Geert Bosch <bosch@gnat.com> writes:
>> For the IA64 port to OpenVMS, we'll be using the Intel Assembler
>> (IAS).
>
> Can you say why?

OpenVMS does not use exactly the same object formats as
other IA64 platforms, and there would be a non-trivial amount
of work to port the GNU assembler. Also, we could encounter
the same issue there: the GNU assembler outputs objects that
can be linked fine by the GNU linker, but not with the OpenVMS
system linker. The we'd go off and port the GNU linker in order
to finally get a working executable. Then, the debugger would
rely on some specific non-standard VMS-bits and we'd have to
port the GNU debugger.

Given the non-UNIXness of VMS, I think it is best to stick
to the first well-specified interface and fix the compiler
to output assembly language that can be understood by the
system assembler. This may also have some benefit for other
IA64 targets, such as GNU/Linux, for which there is also an
IAS port.

   -Geert

Re: Instruction bundling on IA64 and the Intel Assembler

[Zack Weinberg]

kenner@vlsi1.ultra.nyu.edu (Richard Kenner) writes:

>     Always print "{.whatever" in the bundle_selector output template;
>     then, whenever bundling() goes to emit a bundle_selector, have it also
>     look three insns ahead and emit an "}" insn.  I think this will always
>     be right, not interfere with labels and directives, and shouldn't
>     require you to comprehend this problem function.
>
> It still requires *some* comprehension because it's not completely clear
> when those three insns are written out (consider nops).

NOPs have to be considered when determining bundle boundaries, so I think
that the naive three-insns-farther approach will be correct.  (Must
not count things which are not INSN/JUMP_INSN/CALL_INSN of course.)

> However, that still doesn't address part of Geert's comment

... because I have nothing to add to that part.

zw

Re: Instruction bundling on IA64 and the Intel Assembler

[Richard Kenner]

    Always print "{.whatever" in the bundle_selector output template;
    then, whenever bundling() goes to emit a bundle_selector, have it also
    look three insns ahead and emit an "}" insn.  I think this will always
    be right, not interfere with labels and directives, and shouldn't
    require you to comprehend this problem function.

It still requires *some* comprehension because it's not completely clear
when those three insns are written out (consider nops).

However, that still doesn't address part of Geert's comment, which is that
there is little or no documentation in that function and certainly no
overview comments saying what's going on.  Even if there is some simple
way to deal with this issue, that documentation still needs to be provided.

Re: Instruction bundling on IA64 and the Intel Assembler

[Zack Weinberg]

Geert Bosch <bosch@gnat.com> writes:

> On Dec 15, 2003, at 16:39, Zack Weinberg wrote:
>
>> I think you're barking up the wrong tree here.  It seems to me that
>> the right place to handle this is the output template for the
>> bundle_selector insn.  Have this print not just ".mmi", but one of
>> "{.mmi" or "}\n\t{.mmi" depending on whether or not this is the first
>> bundle in the function.  (Set a flag on the very first bundle_selector
>> insn emitted to indicate this.)  Tack a special bundle_selector insn
>> on at the very end of the function that just prints the final close
>> brace.
>
> I thought of this, but I'd think you get into all kinds of
> trouble then with labels and other assembler directives.

Hmm, you are probably right.  Well, how about this?  Always print
"{.whatever" in the bundle_selector output template; then, whenever
bundling() goes to emit a bundle_selector, have it also look three
insns ahead and emit an "}" insn.  I think this will always be right,
not interfere with labels and directives, and shouldn't require you to
comprehend this problem function.

zw

Re: Instruction bundling on IA64 and the Intel Assembler

[Geert Bosch]

On Dec 15, 2003, at 16:39, Zack Weinberg wrote:

> I think you're barking up the wrong tree here.  It seems to me that
> the right place to handle this is the output template for the
> bundle_selector insn.  Have this print not just ".mmi", but one of
> "{.mmi" or "}\n\t{.mmi" depending on whether or not this is the first
> bundle in the function.  (Set a flag on the very first bundle_selector
> insn emitted to indicate this.)  Tack a special bundle_selector insn
> on at the very end of the function that just prints the final close
> brace.

I thought of this, but I'd think you get into all kinds of
trouble then with labels and other assembler directives.

Re: Instruction bundling on IA64 and the Intel Assembler

[Zack Weinberg]

Geert Bosch <bosch@gnat.com> writes:

> For the IA64 port to OpenVMS, we'll be using the Intel Assembler
> (IAS).

Can you say why?

...

>   {.mmi		// use the mmi template for this bundle
>    m inst		// memory instruction
>    ;;			// stop
>    m inst		// memory instruction
>    i inst		// integer instruction
>   }
>
> However, GCC omits the braces and GAS is OK with that. The template
> selector is inserted as an instruction by the function "bundling",
> which seems to be the right place to add the "instruction" that
> closes a bundle.

I think you're barking up the wrong tree here.  It seems to me that
the right place to handle this is the output template for the
bundle_selector insn.  Have this print not just ".mmi", but one of
"{.mmi" or "}\n\t{.mmi" depending on whether or not this is the first
bundle in the function.  (Set a flag on the very first bundle_selector
insn emitted to indicate this.)  Tack a special bundle_selector insn
on at the very end of the function that just prints the final close
brace.

zw

Instruction bundling on IA64 and the Intel Assembler

[Geert Bosch]

For the IA64 port to OpenVMS, we'll be using the Intel Assembler (IAS).
However, currently the assembly code output by GCC is specific to the
GNU assembler (gas) in a few areas. We have identified and fixed most
such parts (patches will follow), but one more tricky issue is
instruction bundling and template selection.

The Itanium Architecture Assembly Language Guide specifies that
template selection directives must appear inside the braces of a bundle,
like this:

  {.mmi		// use the mmi template for this bundle
   m inst		// memory instruction
   ;;			// stop
   m inst		// memory instruction
   i inst		// integer instruction
  }

However, GCC omits the braces and GAS is OK with that. The template
selector is inserted as an instruction by the function "bundling",
which seems to be the right place to add the "instruction" that
closes a bundle. However, this functions is 339 lines, without any
comments on meaning of variables used or reason for tests etc.
It would be very helpful if at least every for-loop would have
an initial comment indicating what it is doing.

There are six places that insert template selection directives,
and I don't know what the difference is between these places what
the state is at these points.

Vladimir, could you give some insights in what this function is
doing, and preferably add some comments? I searched the mailing
lists for more information on the function, but a I didn't find
any details regarding the implementation. If you have links to
old messages explaining things, please let me know.

   -Geert