This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: basic asm and memory clobbers - Proposed solution


Hi,

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.

>>> This is certainly worth mentioning in the 'convert' doc.
>>>
>>> I wonder how often this 'auto-clobber' feature is used, though.  I
>>> don't see it mentioned in the 'interrupt' attribute docs for mep, and
>>> it's not in the basic asm docs either.  If your interrupt doesn't need
>>> many registers, it seems like you'd want to know this and possibly use
>>> extended.  And you'd really want to know if you are doing a
>>> (redundant) push/pop in your interrupt.
>>>
>>>> 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.

You do not have to escape the { and } for extended asm, on this target, 
using
%{ produces even an error.  In deed asm ("pseudo":) expands to nothing, 
it is
probably used as a memory barrier in the .md files.  But asm ("pseudo") 
feeds
"pseudo" to the assembler, which doesn't like it.

BTW: The tilepro target is a32-bit variant of tilegx and it has exactly 
the same logic.

>>>> ia64:  ASM_INPUT emits stop-bits. extended asm does not the same.
>>>> That was already mentioned by Segher.
>>> I already had this one from Segher's email.
>>>
>>> --------------------
>>> Given all this, I'm more convinced than ever of the value of
>>> -Wonly-top-basic-asm.  Basic asm is quirky, has undocumented bits and
>>> its behavior is incompatible with other compilers, even though it uses
>>> the same syntax.  If I had any of this in my projects, I'd sure want
>>> to find it and look it over.
>>>
>>> But maybe Bernd is right and it's best to leave the warning disabled
>>> in v6, even by -Wall.  I may ask this question again in the next phase
>>> 1...
>>>
>> Aehm, yes, maybe the warning could by then be something more reasonable
>> like:
>> "Warning: the semantic of basic asm has changed to include implicit
>> memory clobber,
>> if you think that is a problem for you, please convert it to basic asm,
>> otherwise just relax."
>
> I don't think Jeff wants to pursue changes to basic asm's semantics as 
> part of v6.  I can't blame him, it's a big change for phase 3. So it 
> seems a bit early for the warning to talk about changes in semantics.
>

Yes, Phase 1.


>>> With that in mind, how do you feel about the basic plan:
>>>
>>> - Update the basic asm docs to describe basic asm's current (and
>>> historical) semantics (ie clobber nothing).
>>> - Emphasize how that might be different from users' expectations or
>>> the behavior of other compilers.
>>> - Warn that this could change in future versions of gcc.  To avoid
>>> impacts from this change, use extended.
>>> - Reference the "How to convert from basic asm to extended asm" guide
>>> (where ever it ends up living).
>>> - Mention -Wonly-top-basic-asm as a way to locate affected statements.
>>> - -Wonly-top-basic-asm is disabled by default and not enabled by -Wall
>>> or -Wextra.
>>>
>>> Does this seem like a safe and useful change for v6?
>> 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.


Regards,
Bernd.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]