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

[Richard Sandiford]

peter fuerst <pf@net.alphadv.de> writes:
> [on-list]

Thanks.  For the record, here's the reply I'd sent to Peter privately
on Saturday.

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

OK.

> 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.

Understood.  The final patch would need to do that, of course (as I know
you know from what's below).  The review was trying to point out some of
them.

> 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.

Good.

>> 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.

Sorry, it works the other way.  If you want to add this code, you have
to explain why it's needed.  If you look at GO_IF_LEGITIMATE_ADDRESS,
you'll see that no NEG, SIGN_EXTEND or ZERO_EXTEND addresses are
accepted, and I don't think they have been as long as I've been
working on the MIPS port.  This really should be dead code.

You seem to say similar things about null checks later: i.e. that they
should do no harm.  That isn't really the point.  If they're not needed,
they shouldn't be there at all.

It sounds like may_trap_p isn't a winner, so this special code will
still be in the revised patch.  In that case, you can (should ;) use
mips_classify_address to analyse the address.

>> 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.

OK, just thought I'd check.  Could you clarify how you've arrived at
those conditions then?  If I've understood the problem correctly,
a load or store is potentially problematic if you could be DMAing
to the same address.  Is that right?  And the assumption is that
you would never DMA to stack addresses, so no insns are needed there?
If so, why are constant addresses exempted too?

>> > +#ifndef CACHE_BARRIER_BEFORE_LOAD
>> > ...
>> > +#endif
>> > +#if CACHE_BARRIER_BEFORE_LOAD
>> > ...
>> > +#endif
>
> This will be replaced by a runtime option-switch and evaluating code.

Thanks.

> 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].

OK, but when submitting a patch, please do submit a patch that does what
people need.  There doesn't seem much point reviewing a patch that uses
processor conditionals and an on-off switch if that isn't what you
actually want.

> 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.  ???

I suppose you then couldn't tell the difference between -mfoo= and -mfoo.

But in this sort of situation, where the argument has a fixed set
of possible values, you should be overriding TARGET_HANDLE_OPTION
instead.

>> > +                                                     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.

Not to me it doesn't.  This comment is in a block that handles
SEQUENCEs.  If "this jump is .reorder mode", it won't _be_ a SEQUENCE,
so the code will never be reached.

Maybe we're in violent agreement about what the code should do.  But
this comment implies to me that there's something to fix or clean up,
whereas I don't think there is.

>> 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)

Um, well, you don't seem to have really responded to my last paragraph
above, where I tried to explain the problem with using basic block info
this late.  So I'm a little unsure what you're going to do.  Are you
going to follow the suggestion I made, or do something else?

Richard