On 18.12.2015 10:27, David Wohlferd wrote:
> On 12/17/2015 11:30 AM, Bernd Edlinger wrote:
>> On Thu, 17 Dec 2015 15:13:07, Bernd Schmidt wrote:
>>>> What's your take on making -Wonly-top-basic-asm a default
>>>> (either now or
>>>> v7)? Is making it a non-default a waste of time because no one
>>>> will
>>>> ever see it? Or is making it a default too aggressive? What about
>>>> adding it to -Wall?
>>> Depends if anyone has one in system headers I guess. We could try to
>>> add it to -Wall.
>> Sorry, but if I have to object.
>>
>> Adding this warning to -Wall is too quickly and will bring the ia64,
>> tilegx and mep ports into trouble.
>
> It doesn't look to me like adding the warnings will affect gcc
> itself. But I do see how it could have an impact on people using gcc
> on those platforms, if the warning causes them to convert to extended
> asm.
>
At least we should not start a panic until we have really understood all
the details, how to do that.
>> Each of them invokes some special semantics for basic asm:
>
> I'm collecting these for my "How to convert basic asm to extended asm"
> document. This may need to go in the gcc wiki instead of the user
> guide, since people may find important conversion tips like these
> asynchronous to gcc's releases.
>
>> 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.
> 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.
>> 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."
> 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.
Thanks
Bernd.
