Re: basic asm and memory clobbers - Proposed solution

[David Wohlferd <dw at LimeGreenSocks dot com>]

On 12/20/2015 10:26 AM, Bernd Edlinger wrote:
> On 19.12.2015 19:54, David Wohlferd wrote:
> > > > > mep: mep_interrupt_saved_reg looks for ASM_INPUT in the body, and
> > > > > saves different registers if found.
> > > > I'm trying to follow this code.  A real challenge since I know nothing
> > > > about mep.  But what I see is:
> > > >
> > > > - This routine only applies to functions marked as
> > > > __attribute__((interrupt)).
> > > > - To correctly generate entry/exit, it walks thru each register (up to
> > > > FIRST_PSEUDO_REGISTER) to see if it is used by the routine. If there
> > > > is any basic asm within the routine (regardless of its contents), the
> > > > register is considered 'in use.'
> > > >
> > > > The net result is that every register gets saved/restored by the
> > > > entry/exit of this routine if it contains basic asm.  The reason this
> > > > is important is that if someone just adds a colon, it would suddenly
> > > > *not* save/restore all the registers.  Depending on what the asm does,
> > > > this could be bad.
> > > >
> > > > Does that sound about right?
> > > Yes.
> > Seems like a doc update would be appropriate here too then, if anyone
> > wanted to volunteer...
> To confirm this, I built a cross-compiler, but It was difficult, because
> of pr64402.
> Yes, a function with __attribute__((interrupt)) spills lots of
> registers, when it just
> contains asm(""); but almost nothing, if asm("":); is used.  That is
> remarkable.

So if you use extended and clobber a couple registers asm("":::"r0", 
"r1"), does it spill just those specific registers?  That would be cool.

I don't write interrupt handlers, and certainly not on mep.  But the 
little I know about them says that performance is an important (and 
sometimes critical) characteristic.  There would be risk in changing 
this to extended (if you used a register but forgot to clobber it), but 
depending on the interrupt, it could be a nice performance 'win.'

If no one else is prepared to step up to write this, I can.  I'm just 
uncomfortable doing so since I can't try it myself.  And I feel weird 
writing a patch for mep given that I know nothing about it.

But since Bernd has tried it, maybe something like this added to the 
'interrupt' attribute on 
https://gcc.gnu.org/onlinedocs/gcc/MeP-Function-Attributes.html
---------
Be aware that if the function contains any basic @code{asm} 
(@pxref{Basic Asm}), all registers (whether referenced in the asm or 
not) will be preserved upon entry and restored upon exiting the 
interrupt. More efficient code can be generated by using extended 
@code{asm} (@pxref{Extended Asm}) and explicitly listing only the 
specific registers that need to be preserved (or none if your asm 
preserves any registers it uses).
---------

> > > > > tilegx:  They never wrap {} around inline asm. But extended asm, are
> > > > > handled differently, probably automatically surrounded by braces.
> > > > I know nothing about tilegx either.  I've tried to read the code, and
> > > > it seems like basic asm does not get 'bundled' while extended might be.
> > > >
> > > > Bundling for tilegx (as I understand it) is when you explicitly fill
> > > > multiple pipelines by doing something like this:
> > > >
> > > >      { add r3,r4,r5 ; add r7,r8,r9 ; lw r10,r11 }
> > > >
> > > > So if you have a basic asm statement, you wouldn't want it
> > > > automatically bundled by the compiler, since your asm could be more
> > > > than 3 statements (the max?).  Or your asm may do its own bundling. So
> > > > it makes sense to never output braces when outputting basic asm.
> > > >
> > > > I know I'm guessing about what this means, but doesn't it seem like
> > > > those same limitations would apply to extended?  I wonder if this is a
> > > > bug.  I don't see any discussion of bundling (of this sort) in the
> > > > docs.
> > > I wold like to build a cross compiler, but currently that target seems
> > > to be broken. I have to check
> > > that target anyways, because of my other patch with the memory clobbers.
> > >
> > > I see in tilegx_asm_output_opcode, that they do somehow automatically
> > > place braces.
> > > An asm("pseudo":) has a special meaning, and can be replaced with ""
> > > or "}".
> > > However the static buf[100] means that any extended asm string > 95
> > > characters, invokes
> > > the gcc_assert in line 5487.  In the moment I would _not_ recommend
> > > changing any asm
> > > statements without very careful testing.
> > Yes, the handling of extended asm there is very strange.
> >
> > If bundles can only be 3 instructions, then appending an entire
> > extended asm in a single (already in-use) bundle seems odd.  But maybe
> > that's just because I don't understand tilegx.
> >
> > I'm not sure it's just changing basic asm to extended I would be
> > concerned about.  I'd worry about using extended asm at all...
> I also built a tilegx cross compiler, but that was even more difficult,
> because of pr68917, which hit me when building the libgcc.
>
> I tried a while, but was unable to find any example of an extended asm
> that gets
> auto-braces, or which can trigger the mentioned gcc_assert. It looks
> like, in all
> cases the bundles are closed already before the extended asm gets scheduled.
> Probably the middle end already knows that it can't parse the asm string.
> I see examples of extended asm in the linux /arch/tile tree, and some have
> braces, some not.  But as it seems, they are completely left alone by
> the expansion.

So what do the docs need to say here?  Basic asm never wraps with {}, 
but apparently extended doesn't either.  So, there's nothing to say?

> You do not have to escape the { and } for extended asm, on this target,
> using %{ produces even an error.

I believe the only the only target that needs to escape {} is i386, 
since it's the only one that supports dialects (att & intel).

> In deed asm ("pseudo":) expands to nothing, it is
> probably used as a memory barrier in the .md files.

Yeah, that's what it looked like to me too.

> > > Yes, but please add a test case with some examples where the warning is
> > > expected to be triggered,
> > > and where it is not.
> > I have one, but I haven't added the dejagnu stuff to it yet.  I'm not
> > really an expert there and could use some help.  Simple things like
> > where does the file go and what should it be called.
> Look at gcc/testsuite/c-c++-common/W*.c  for examples how to do that.
> I think, this would be a good place/naming convention for your test case
> too.

Given that c and cpp use different parsers, I'll want to create tests 
for both (well, same tests, different extensions).  Should both 
Wonly-top-basic-asm.c and Wonly-top-basic-asm.cpp go in that directory?  
There aren't any other cpp files there.

dw