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.