Patch for automaton based pipeline hazard recognizer (part #2).

[Bernd Schmidt]

On Wed, 31 Jan 2001, Vladimir Makarov wrote:

> This is the 1st part of file genautomata.c:

Some preliminary comments about this code.  (I made basically the same points
a long time ago before this code was checked into the Cygnus repository.)

In general, this file is too big.  There appear to be some independent parts
which could be split up.  If we bother with abstract data types in the way
this code does, then each one should ideally go into its own file (and ideally
all of it should go into its own directory, but I believe there are Makefile
issues when working with subdirs).

There is no commentary that helps one understand what exactly is going
on - a rough outline of how everything fits together, which steps are
taken when processing the input, etc.  This sort of documentation is
vital.  For example, instead of

> /* This page contains checker of pipeline hazard description. */

there should be a comment describing exactly what the checker has to do and
why.

> /*====All the folowing code is pipeline hazard description translator.=======*/

The coding standards disallow fancy comment formatting like this, I believe.

> #ifdef HAVE_ASSERT_H
> #include <assert.h>
> #else
> #ifndef assert
> #define assert(code) do { if ((code) == 0) abort ();} while (0)
> #endif
> #endif

I believe gcc source code can't use assert.h, since there's a risk of picking
up our own assert.h file.  There used to be a comment about this in
dwarf2out.c:

  /* We cannot use <assert.h> in GCC source, since that would include
     GCC's assert.h, which may not be compatible with the host compiler.  */

Use if statements and abort () instead.

> /* The following structure represents variable length array (vla) of
>    pointers and HOST WIDE INTs.  See commentaries for macros working
>    with vla. */
> typedef struct {
>   size_t length;      /* current size of vla. */
>   varray_type varray; /* container for vla. */
> } vla_ptr_t;
>
> typedef vla_ptr_t vla_hwint_t;

A lot of code in this file simply appears to wrap existing data structures
and giving them new names.  I think this is a bad idea; people familiar
with other parts of gcc know what a varray is, but they haven't got a clue
what a vla_ptr_t or vla_hwint_t stands for.
I believe all of this wrapping code should go.

> /* Macros for work with internal representation (IR) storage
>    manager. */
>
> /* Start work with IR storage manager. */
> #define IR_START_ALLOC()    obstack_init (&irp)
>
> /* Finish work with the storage manager. */
> #define IR_STOP_ALLOC()     obstack_free (&irp, NULL)

Likewise.  IMO nothing is gained by these macros; people know how to deal
with obstacks and this just adds an unfamiliar interface to them.

> /* Standard designators for true and false values. */
> #define FALSE 0
> #define TRUE  1

I'm no big fan of these either, and I believe we generally use 0 and 1 in
the rest of the gcc sources.

> /* Options with the following names can be set up in automata_option
>    construction. */
>
> #define NO_MINIMIZATION_OPTION "-no-minimization"
>
> #define W_OPTION "-w"
>
> #define NDFA_OPTION "-ndfa"

Likewise - why have a macro for everything?  I don't see the point; these
are only used once, right?  If you absolutely need these macroized, at
least put the macros close to their use (about 1000 lines further down).
But I'd really like to see them disappear.

> /* The function checks that NAME does not contain ". */

Unterminated string.

> /* The function fixes warnings/errors `... already declared'
>    `... repeated declaration'.  The function also fixes occurences of
>    undeclared automata name in unit declarations or absence of insn
>    name in unit declarations if there are automaton declarations.  The
>    function checks bypasses and forms list of bypasses for each
>    (output) insn. */
> static void
> process_decls ()
> {

In this function, a comment before each for loop would help readability.

> /* The page contains abstract data `ticker'. */

As I said when I replied to the first part - do we really need this?
(I might object less if it disappeared into a separate file).

> /* The page contains macros for work with bits strings. */
>
> /* Set bit number bitno in the bit string.  The macro is not side
>    effect proof. */
> #define SET_BIT(bitstring, bitno)					  \
>   (((char *) (bitstring)) [(bitno) / CHAR_BIT] |= 1 << (bitno) % CHAR_BIT)
>
> /* Test if bit number bitno in the bitstring is set.  The macro is not
>    side effect proof. */
> #define TEST_BIT(bitstring, bitno)                                        \
>   (((char *) (bitstring)) [(bitno) / CHAR_BIT] >> (bitno) % CHAR_BIT & 1)

Should use bitmap or sbitmap code.


Bernd