Re: [PATCH]: Add special code generation for SGI IP28. (fwd)

[peter fuerst <pf at net dot alphadv dot de>]

[on-list]

---------- Forwarded message ----------
Date: Sat, 4 Mar 2006 02:50:40 +0100 (CET)
From: peter fuerst <pf@Indigo2.Peter>
To: Richard Sandiford <richard@codesourcery.com>
Subject: Re: [PATCH]: Add special code generation for SGI IP28.


[off-list]

Thank you for reviewing the patch so promptly!

I'll submit a revised patch and copyright assignment - hopefully soon,
so for now only some preliminary explanations:

This work started in September 2004 with gcc 2.95.4, and i kept the parts
dealing with instruction-checking identical across 2.95.4, 3.4 and 4 since
then, so some of this stuff may not utilize the latest methods.

The FOR_EACH_BB... parts where developed on 4.0.1/2.


> > +    case PLUS: case MINUS: /* always `SP + const' ? */
> > +      if (GET_CODE (XEXP (x, 1)) == REG
> > +          && REGNO (XEXP (x, 1)) == STACK_POINTER_REGNUM)
> > +        return 0;
           /* no break! */
> ...
> Did you check the compiler output to make sure that this special case
> was working?  The reason I ask is that in SP + const, the register will
> always come first, so you want XEXP (x, 0) rather than XEXP (x, 1) here.
> I don't see how this case could trigger as-is.

Yes, the index changed between 2.95.4 and 3.x, so, for the reason mentioned
above, i kept checks for both (x, 1) and (x, 0). The one for (x, 1) can of
course go away in any patch submitted for the future.

> SP + const should always be a PLUS, not a MINUS, so the MINUS case seems
> redundant.
> ...
> > +    case NEG: case SIGN_EXTEND: case ZERO_EXTEND:
> I don't understand these cases.  When can SIGN_EXTEND, ZERO_EXTEND
> and NEG occur as addresses?

Have to search the old RTL-dumps to remember whether this was a real issue
or just precaution.  Anyway, abandoning these checks would just increase the
number of unnecessary cache-barriers in worst case.

> If so, you can just use may_trap_p instead.  If not, then...

At a first inspection, it seems may_trap_p() doesn't fit the needs well.

> > +#ifdef TARGET_IP28

This is a mere relic of former attempts to allow disabling the whole
cache-barrier stuff at compile time.

> > +#ifndef CACHE_BARRIER_BEFORE_LOAD
> > ...
> > +#endif
> > +#if CACHE_BARRIER_BEFORE_LOAD
> > ...
> > +#endif

This will be replaced by a runtime option-switch and evaluating code.
The people, compiling for IP32 (SGI's O2) with R10k, are not done with
the simple on/off-switch submitted in mips.opt-diff, they need to enable
cache-barriers before loads & stores on the commandline, with something
like -mr10k-cache-barrier[=X].

BTW: the "options" documentation states:
"Otherwise, if the option takes an argument, var is a pointer to the
 argument string. The pointer will be null if the argument is optional
                              ^^^^^^^^^^^^
 and wasn't given."
To my understanding, this means, omitting an optional argument to a command-
line-option gives the same result as omitting this option altogether.  Not
very helpfull, an empty string instead seems appropriate.  ???

> > +                                                     If we could know in
> > +             advance that this jump is in `.reorder' mode, where gas will
> > +             insert a `nop' into the delay-slot, we could skip this test.
> > + ...
> I don't follow this comment.  If the call or jump is in "reorder" mode,
> it won't have any delay slots.

That's, what this sentence (maybe superfluously) says.

> You have a lot of hacks to do with the basic block info.
> For example, the "0 <= bb->index" check should never have been
> needed in a FOR_EACH_BB loop, even in the days when the entry
> and exit blocks had negative indexes.  FOR_EACH_BB should
> skip those blocks anyway.  The check should be completely
> redundant now, because even the entry and exit blocks have
> positive indexes.
>
> Second, you keep checking for basic block notes in the middle of blocks.
Stumbled over these notes and labels and blocks-inside-blocks (sigh), when
tracing the basic block scan.
> I think this is because dbr_schedule does not keep basic block
> information up-to-date.  It would obviously be nice if it did,
> but in this case, it doesn't really hinder you much.  You can
> just walk from get_insns () to NULL and check for labels, much
> like the mips_avoid_hazards and vr4130_align_insns functions do.

There may be one or another superfluous check for null-insn or the like, but
mostly the checks just reflect my experience, when trying to implement the
basic block scan. Too often there was either unintended (and unexpected)
assembly output or gcc segfaulted, when i trusted the documentation.
(Maybe it's much better now in 4.2, i didn't try to open the same pitfalls
again)

Shortly recapitulated, what all this stuff should do:
Insert a cache-barrier at the begin of each basic block (return from a call
also must be regarded as begin of a bb in this context), which contains a
memory access (store only or store & load for some machine), other than via
stackpointer or explicit constant, thus cutting off any speculative path to
this memory access, since mis-speculation could have ugly effects on the
caches (etc...).  And try to avoid emitting too many unnecessary cache-
barriers in this process.

kind regards

pf


On Fri, 3 Mar 2006, Richard Sandiford wrote:

> Date: Fri, 03 Mar 2006 14:19:02 +0000
> From: Richard Sandiford <richard@codesourcery.com>
> To: pf@net.alphadv.de
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH]: Add special code generation for SGI IP28.
>
>