| Message ID | AM4PR07MB157101A1194D2A45C841E506E47C0@AM4PR07MB1571.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On 5/5/2016 10:29 AM, Bernd Edlinger wrote: > Hi! > > this patch is inspired by recent discussion about basic asm: > > Currently a basic asm is an instruction scheduling barrier, > but not a memory barrier, and most surprising, basic asm > does _not_ implicitly clobber CC on targets where > extended asm always implicitly clobbers CC, even if > nothing is in the clobber section. > > This patch makes basic asm implicitly clobber CC on certain > targets, and makes the basic asm implicitly clobber memory, > but no general registers, which is what could be expected. > > This is however only done for basic asm with non-empty > assembler string, which is in sync with David's proposed > basic asm warnings patch. > > Due to the change in the tree representation, where > ASM_INPUT can now be the first element of a > PARALLEL block with the implicit clobber elements, > there are some changes necessary. > > Most of the changes in the middle end, were necessary > because extract_asm_operands can not be used to find out > if a PARALLEL block is an asm statement, but in most cases > asm_noperands can be used instead. > > There are also changes necessary in two targets: pa, and ia64. > I have successfully built cross-compilers for these targets. > > Boot-strapped and reg-tested on x86_64-pc-linux-gnu > OK for trunk? A few questions: 1) I'm not clear precisely what problem this patch fixes. It's true that some people have incorrectly assumed that basic asm clobbers memory and this change would fix their code. But some people also incorrectly assume it clobbers registers. I assume that's why Jeff Law proposed making basic asm "an opaque blob that read/write/clobber any register or memory location." Do we have enough problem reports from users to know which is the real solution here? 2) The -Wbasic-asm warning patch wasn't approved for v6. If we are going to change this behavior now, is it time? 3) I assume there are good reasons why extended asm can't be used at top level. Will adding these clobbers cause those problems in basic asm too? 4) There are more basic asm docs that need to change: "It also does not know about side effects of the assembler code, such as modifications to memory or registers. Unlike some compilers, GCC assumes that no changes to either memory or registers occur. This assumption may change in a future release." dw
On 05/06/16 08:35, David Wohlferd wrote: > On 5/5/2016 10:29 AM, Bernd Edlinger wrote: >> Hi! >> >> this patch is inspired by recent discussion about basic asm: >> >> Currently a basic asm is an instruction scheduling barrier, >> but not a memory barrier, and most surprising, basic asm >> does _not_ implicitly clobber CC on targets where >> extended asm always implicitly clobbers CC, even if >> nothing is in the clobber section. >> >> This patch makes basic asm implicitly clobber CC on certain >> targets, and makes the basic asm implicitly clobber memory, >> but no general registers, which is what could be expected. >> >> This is however only done for basic asm with non-empty >> assembler string, which is in sync with David's proposed >> basic asm warnings patch. >> >> Due to the change in the tree representation, where >> ASM_INPUT can now be the first element of a >> PARALLEL block with the implicit clobber elements, >> there are some changes necessary. >> >> Most of the changes in the middle end, were necessary >> because extract_asm_operands can not be used to find out >> if a PARALLEL block is an asm statement, but in most cases >> asm_noperands can be used instead. >> >> There are also changes necessary in two targets: pa, and ia64. >> I have successfully built cross-compilers for these targets. >> >> Boot-strapped and reg-tested on x86_64-pc-linux-gnu >> OK for trunk? > > A few questions: > > 1) I'm not clear precisely what problem this patch fixes. It's true > that some people have incorrectly assumed that basic asm clobbers memory > and this change would fix their code. But some people also incorrectly > assume it clobbers registers. I assume that's why Jeff Law proposed > making basic asm "an opaque blob that read/write/clobber any register or > memory location." Do we have enough problem reports from users to know > which is the real solution here? > Whenever I do something for gcc I do it actually for myself, in my own best interest. And this is no exception. The way I see it, is this: in simple cases a basic asm behaves as if it would clobber memory, because of the way Jeff implemented the asm handling in sched-deps.c some 20 years ago. Look for ASM_INPUT where we have this comment: "Traditional and volatile asm instructions must be considered to use and clobber all hard registers, all pseudo-registers and all of memory." The assumption that it is OK to clobber memory in a basic asm will only break if the asm statement is inlined in a loop, and that may happen unexpectedly, when gcc rolls out new optimizations. That's why I consider this to be security relevant. But OTOH you see immediately that all general registers are in use by gcc, unless you declare a variable like register int eax __asm__("rax"); then it is perfectly OK to use rax in a basic asm of course. And if we want to have implicitly clobbered registers, like the diab compiler handles the basic asm, then this patch will make it possible to add a target hook that clobbers additional registers for basic asm. > 2) The -Wbasic-asm warning patch wasn't approved for v6. If we are > going to change this behavior now, is it time? > Yes. We have stage1 for gcc-7 development, I can't think of a better time for it. I would even say, the -Wbasic-asm warning patch makes more sense now, because we could warn, that the basich asm clobbers memory, which it did not previously. > 3) I assume there are good reasons why extended asm can't be used at top > level. Will adding these clobbers cause those problems in basic asm too? > No, these don't come along here, and nothing should change for them. > 4) There are more basic asm docs that need to change: "It also does not > know about side effects of the assembler code, such as modifications to > memory or registers. Unlike some compilers, GCC assumes that no changes > to either memory or registers occur. This assumption may change in a > future release." > Yes, I should change that sentence too. Maybe this way: "Unlike some compilers, GCC assumes that no changes to registers occur. This assumption may change in a future release." Thanks Bernd. > dw
>> A few questions: >> >> 1) I'm not clear precisely what problem this patch fixes. It's true >> that some people have incorrectly assumed that basic asm clobbers memory >> and this change would fix their code. But some people also incorrectly >> assume it clobbers registers. I assume that's why Jeff Law proposed >> making basic asm "an opaque blob that read/write/clobber any register or >> memory location." Do we have enough problem reports from users to know >> which is the real solution here? >> > Whenever I do something for gcc I do it actually for myself, in my own > best interest. And this is no exception. Seems fair. You are the one putting the time in to change it. But do you have actual code that is affected by this? You can't really be planning to wait until v7 is released to have your projects work correctly? > The way I see it, is this: in simple cases a basic asm behaves as if > it would clobber memory, because of the way Jeff implemented the > asm handling in sched-deps.c some 20 years ago. > > Look for ASM_INPUT where we have this comment: > "Traditional and volatile asm instructions must be considered to use > and clobber all hard registers, all pseudo-registers and all of > memory." > > The assumption that it is OK to clobber memory in a basic asm will only > break if the asm statement is inlined in a loop, and that may happen > unexpectedly, when gcc rolls out new optimizations. > That's why I consider this to be security relevant. I'm not sure I follow. Do you fear that gcc could mistakenly move the asm into a nearby loop during optimization (resulting in who-knows-what results)? Or is there some way that any basic asm in a loop could have some kind of a problem? > But OTOH you see immediately that all general registers are in use > by gcc, unless you declare a variable like > register int eax __asm__("rax"); > then it is perfectly OK to use rax in a basic asm of course. According to the docs, that is only supported for global registers. The docs for local register variables explicitly say that it can't be used as input/outputs for basic asm. > And if we want to have implicitly clobbered registers, like the > diab compiler handles the basic asm, then this patch will > make it possible to add a target hook that clobbers additional > registers for basic asm. I think we should try to avoid changing the semantics in v7 for memory and then changing them again in v8 for registers. IOW, if I see some basic asm in a project (or on stack overflow/blog as a code fragment), should I assume it was intended for v6 semantics? v7? v8? People often copy this stuff without understanding what it does. The more often the semantics change, the harder it is to use correctly and maintain. >> 2) The -Wbasic-asm warning patch wasn't approved for v6. If we are >> going to change this behavior now, is it time? >> > Yes. We have stage1 for gcc-7 development, I can't think of a better > time for it. > I would even say, the -Wbasic-asm warning patch makes more sense now, > because we could warn, that the basich asm clobbers memory, which it > did not previously. After your patch has been checked in, I'll re-submit this. >> 4) There are more basic asm docs that need to change: "It also does not >> know about side effects of the assembler code, such as modifications to >> memory or registers. Unlike some compilers, GCC assumes that no changes >> to either memory or registers occur. This assumption may change in a >> future release." >> > Yes, I should change that sentence too. > > Maybe this way: > > "Unlike some compilers, GCC assumes that no changes to registers > occur. This assumption may change in a future release." Is it worth describing the fact that the semantics have changed here? Something like "Before v7, gcc assumed no changes were made to memory." I guess users can "figure it out" by reading the v6 docs and comparing it to v7. But if the semantic change has introduced a problem that someone is trying to debug, this could help them track it down. Also, I'm kind of hoping that v7 is the 'future release.' If we are going to change the clobbers, I'd hope that we'd do it all at one time, rather than spreading it out across several releases. If no one is prepared to step up and implement (or at least defend) the idea of clobbering registers, I'd like to see the "This assumption may change in a future release" part removed. Since (theoretically) anything can change at any time, the only reason this text made sense was because a change was imminent. If that's no longer true, it's time for it to go. dw
On 05/07/16 01:33, David Wohlferd wrote: > >>> A few questions: >>> >>> 1) I'm not clear precisely what problem this patch fixes. It's true >>> that some people have incorrectly assumed that basic asm clobbers memory >>> and this change would fix their code. But some people also incorrectly >>> assume it clobbers registers. I assume that's why Jeff Law proposed >>> making basic asm "an opaque blob that read/write/clobber any register or >>> memory location." Do we have enough problem reports from users to know >>> which is the real solution here? >>> >> Whenever I do something for gcc I do it actually for myself, in my own >> best interest. And this is no exception. > > Seems fair. You are the one putting the time in to change it. > > But do you have actual code that is affected by this? You can't really > be planning to wait until v7 is released to have your projects work > correctly? > No, I can wait. This is a long term investment in overall software reliability. >> The way I see it, is this: in simple cases a basic asm behaves as if >> it would clobber memory, because of the way Jeff implemented the >> asm handling in sched-deps.c some 20 years ago. >> >> Look for ASM_INPUT where we have this comment: >> "Traditional and volatile asm instructions must be considered to use >> and clobber all hard registers, all pseudo-registers and all of >> memory." >> >> The assumption that it is OK to clobber memory in a basic asm will only >> break if the asm statement is inlined in a loop, and that may happen >> unexpectedly, when gcc rolls out new optimizations. >> That's why I consider this to be security relevant. > > I'm not sure I follow. Do you fear that gcc could mistakenly move the > asm into a nearby loop during optimization (resulting in who-knows-what > results)? Or is there some way that any basic asm in a loop could have > some kind of a problem? > It is a risk, that who-knows-what will happen, unexpectedly. Bernd. >> But OTOH you see immediately that all general registers are in use >> by gcc, unless you declare a variable like >> register int eax __asm__("rax"); >> then it is perfectly OK to use rax in a basic asm of course. > > According to the docs, that is only supported for global registers. The > docs for local register variables explicitly say that it can't be used > as input/outputs for basic asm. > >> And if we want to have implicitly clobbered registers, like the >> diab compiler handles the basic asm, then this patch will >> make it possible to add a target hook that clobbers additional >> registers for basic asm. > > I think we should try to avoid changing the semantics in v7 for memory > and then changing them again in v8 for registers. > > IOW, if I see some basic asm in a project (or on stack overflow/blog as > a code fragment), should I assume it was intended for v6 semantics? v7? > v8? People often copy this stuff without understanding what it does. > The more often the semantics change, the harder it is to use correctly > and maintain. > >>> 2) The -Wbasic-asm warning patch wasn't approved for v6. If we are >>> going to change this behavior now, is it time? >>> >> Yes. We have stage1 for gcc-7 development, I can't think of a better >> time for it. >> I would even say, the -Wbasic-asm warning patch makes more sense now, >> because we could warn, that the basich asm clobbers memory, which it >> did not previously. > > After your patch has been checked in, I'll re-submit this. > >>> 4) There are more basic asm docs that need to change: "It also does not >>> know about side effects of the assembler code, such as modifications to >>> memory or registers. Unlike some compilers, GCC assumes that no changes >>> to either memory or registers occur. This assumption may change in a >>> future release." >>> >> Yes, I should change that sentence too. >> >> Maybe this way: >> >> "Unlike some compilers, GCC assumes that no changes to registers >> occur. This assumption may change in a future release." > > Is it worth describing the fact that the semantics have changed here? > Something like "Before v7, gcc assumed no changes were made to memory." > I guess users can "figure it out" by reading the v6 docs and comparing > it to v7. But if the semantic change has introduced a problem that > someone is trying to debug, this could help them track it down. > > Also, I'm kind of hoping that v7 is the 'future release.' If we are > going to change the clobbers, I'd hope that we'd do it all at one time, > rather than spreading it out across several releases. > > If no one is prepared to step up and implement (or at least defend) the > idea of clobbering registers, I'd like to see the "This assumption may > change in a future release" part removed. Since (theoretically) > anything can change at any time, the only reason this text made sense > was because a change was imminent. If that's no longer true, it's time > for it to go. > > dw
On 06/05/16 07:35, David Wohlferd wrote: > 1) I'm not clear precisely what problem this patch fixes. It's true > that some people have incorrectly assumed that basic asm clobbers > memory and this change would fix their code. But some people also > incorrectly assume it clobbers registers. I assume that's why Jeff > Law proposed making basic asm "an opaque blob that > read/write/clobber any register or memory location." A few more things: Jeff Law did propose this, but it's impossible to do because it inevitably causes reload failures. My argument in support of Bernd's proposal is that it makes sense from a *practical* software reliability point of view. It wouldn't hurt, and might fix some significant bugs. It's similar to the targets which always implicitly clobber "cc". It corresponds to what I always assumed basic asm did, and I'm sure that I'm not alone. This change might fix some real bugs and it is extremely unlikely to break anything. Andrew.
On Thu, 5 May 2016, Bernd Edlinger wrote: > Hi! > > this patch is inspired by recent discussion about basic asm: > > Currently a basic asm is an instruction scheduling barrier, > but not a memory barrier, and most surprising, basic asm > does _not_ implicitly clobber CC on targets where > extended asm always implicitly clobbers CC, even if > nothing is in the clobber section. > > This patch makes basic asm implicitly clobber CC on certain > targets, and makes the basic asm implicitly clobber memory, > but no general registers, which is what could be expected. > > This is however only done for basic asm with non-empty > assembler string, which is in sync with David's proposed > basic asm warnings patch. > > Due to the change in the tree representation, where > ASM_INPUT can now be the first element of a > PARALLEL block with the implicit clobber elements, > there are some changes necessary. > > Most of the changes in the middle end, were necessary > because extract_asm_operands can not be used to find out > if a PARALLEL block is an asm statement, but in most cases > asm_noperands can be used instead. > > There are also changes necessary in two targets: pa, and ia64. > I have successfully built cross-compilers for these targets. > > Boot-strapped and reg-tested on x86_64-pc-linux-gnu > OK for trunk? I'm generally sympathetic with the change but I wonder if it would make sense to re-write "basic asm" into general asms to not need to special case them. I'd do that during gimplification for example. At least it sounds to me that its semantics can be fully expressed with generic asms? (Maybe apart from the only-if-ASM_STRING-is-empty part) Thanks, Richard.
On 05/09/16 09:56, Richard Biener wrote: > On Thu, 5 May 2016, Bernd Edlinger wrote: > >> Hi! >> >> this patch is inspired by recent discussion about basic asm: >> >> Currently a basic asm is an instruction scheduling barrier, >> but not a memory barrier, and most surprising, basic asm >> does _not_ implicitly clobber CC on targets where >> extended asm always implicitly clobbers CC, even if >> nothing is in the clobber section. >> >> This patch makes basic asm implicitly clobber CC on certain >> targets, and makes the basic asm implicitly clobber memory, >> but no general registers, which is what could be expected. >> >> This is however only done for basic asm with non-empty >> assembler string, which is in sync with David's proposed >> basic asm warnings patch. >> >> Due to the change in the tree representation, where >> ASM_INPUT can now be the first element of a >> PARALLEL block with the implicit clobber elements, >> there are some changes necessary. >> >> Most of the changes in the middle end, were necessary >> because extract_asm_operands can not be used to find out >> if a PARALLEL block is an asm statement, but in most cases >> asm_noperands can be used instead. >> >> There are also changes necessary in two targets: pa, and ia64. >> I have successfully built cross-compilers for these targets. >> >> Boot-strapped and reg-tested on x86_64-pc-linux-gnu >> OK for trunk? > > I'm generally sympathetic with the change but I wonder if it would > make sense to re-write "basic asm" into general asms to not > need to special case them. I'd do that during gimplification > for example. > > At least it sounds to me that its semantics can be fully expressed > with generic asms? (Maybe apart from the only-if-ASM_STRING-is-empty > part) > That was also my first idea too. In simple cases an asm ("whatever"); should do the same as asm ("whatever" ::: ); Adding a "memory" to the clobber list would be simple that's true. But in general it can be pretty complicated, especially if the string contains the special characters % { | }. For example asm ("#%") is OK, but asm ("#%" :) is an ERROR. So, single % must be duplicated, that may be a general rule (hopefully). On some targets { | } mean different things, and must be escaped, but on other targets these must not be escaped. So that is target dependent. Some targets replace whatever they want with the ASM_OUTPUT_OPCODE hook. Example: i386 replaces "%v" with "v" or "", tilegx replaces "pseudo" with "", this hook is only called with extended asm. And we must know what it does and reverse it's effect. Here it starts to become difficult. And the ia64 target have different semantics for basic asm than without, i.e. they always emit stop bits for traditional asms. So they make a difference between extended and basic asm. I have really no idea how to do this when extended and basic asm have exactly the same tree structure. That's why I thought I can as well add the clobbers to the basic asm's tree representation. Thanks Bernd. > Thanks, > Richard. >
On 05/09/2016 03:37 PM, Bernd Edlinger wrote: > On 05/09/16 09:56, Richard Biener wrote: >> >> At least it sounds to me that its semantics can be fully expressed >> with generic asms? (Maybe apart from the only-if-ASM_STRING-is-empty >> part) >> > > That was also my first idea too. > > In simple cases an asm ("whatever"); should do the same as > asm ("whatever" ::: ); > > Adding a "memory" to the clobber list would be simple that's true. > > But in general it can be pretty complicated, especially if the > string contains the special characters % { | }. Is the only difference in how the string is output? Maybe we can have a slightly different form of ASM_OPERANDS (with a bit set, or with the string wrapped in something else) to indicate that it's old-style. Bernd
On 05/09/16 15:46, Bernd Schmidt wrote: > On 05/09/2016 03:37 PM, Bernd Edlinger wrote: >> On 05/09/16 09:56, Richard Biener wrote: >>> >>> At least it sounds to me that its semantics can be fully expressed >>> with generic asms? (Maybe apart from the only-if-ASM_STRING-is-empty >>> part) >>> >> >> That was also my first idea too. >> >> In simple cases an asm ("whatever"); should do the same as >> asm ("whatever" ::: ); >> >> Adding a "memory" to the clobber list would be simple that's true. >> >> But in general it can be pretty complicated, especially if the >> string contains the special characters % { | }. > > Is the only difference in how the string is output? Maybe we can have a > slightly different form of ASM_OPERANDS (with a bit set, or with the > string wrapped in something else) to indicate that it's old-style. Most of the difference is what happens in final.c, and adding a new attribute to the ASM_OPERANDS tree node is definitely one option. I tried to implement it in a way that causes the least confusion. There are lots of different tree representations for an extended asm statement in genereal, but only one for a basic asm. An extended asm that has no outputs and no clobbers, is an ASM_OPERAND node with optional vector of ASM_INPUTs containig the input constraint: ASM_OPERAND { "asm", "", 0, VEC { inputs...}, VEC { ASM_INPUT ("x")...} but if it has any CLOBBERS, it will look like this: PARALLEL { ASM_OPERAND, CLOBBER... } if it has one output, and zero clobbers we have: SET { x, ASM_OPERAND } and in case we have more than one output we have: PARALLEL { SET { x, ASM_OPERAND }... , CLOBBER... } A basic asm is just an ASM_INPUT that is not underneath an ASM_OPERAND. But to add any CLOBBERs to this ASM_INPUT it has to be in PARALLEL with the CLOBBERs, so that would look like this: PARALLEL { ASM_INPUT{ "asm" }, CLOBBER... } There are lots of places where we need to know if a statement is an assembler statement, in most places this is done in this way: GET_CODE (PATTERN (insn)) == ASM_INPUT || asm_noperands (PATTERN (insn)) >= 0 There are a handful of places where it is done it this way: GET_CODE (PATTERN (insn)) == ASM_INPUT || extract_asm_operands (PATTERN (insn)) != NULL_RTX extract_asm_operands locates the ASM_OPERAND node from an extended asm that can have either of the several forms above, but in most cases the result is not looked at. Making extract_asm_operands return anything but an ASM_OPERANDS is impossible, but making asm_noperands return 0 for a PARALLEL { ASM_INPUT, CLOBBER... } is not too complicated. Fortunately, all the remaining uses of extract_asm_operands really mean an extended asm. Hope that explains my idea. Thanks Bernd.
On 05/07/2016 11:38 AM, Andrew Haley wrote: > On 06/05/16 07:35, David Wohlferd wrote: > >> 1) I'm not clear precisely what problem this patch fixes. It's true >> that some people have incorrectly assumed that basic asm clobbers >> memory and this change would fix their code. But some people also >> incorrectly assume it clobbers registers. I assume that's why Jeff >> Law proposed making basic asm "an opaque blob that >> read/write/clobber any register or memory location." > > A few more things: > > Jeff Law did propose this, but it's impossible to do because it > inevitably causes reload failures. Right. > > My argument in support of Bernd's proposal is that it makes sense from > a *practical* software reliability point of view. It wouldn't hurt, > and might fix some significant bugs. It's similar to the targets > which always implicitly clobber "cc". It corresponds to what I always > assumed basic asm did, and I'm sure that I'm not alone. This change > might fix some real bugs and it is extremely unlikely to break > anything. And by making basic asms use/clobber memory in particular, it means code using them is less likely to break as the optimizers continue to get smarter about memory loads/stores. I haven't gone through the actual patch yet, but I like it's basic goals. Jeff
On 5/18/2016 3:07 PM, Jeff Law wrote: > On 05/07/2016 11:38 AM, Andrew Haley wrote: >> My argument in support of Bernd's proposal is that it makes sense from >> a *practical* software reliability point of view. It wouldn't hurt, >> and might fix some significant bugs. It's similar to the targets >> which always implicitly clobber "cc". It corresponds to what I always >> assumed basic asm did, and I'm sure that I'm not alone. This change >> might fix some real bugs and it is extremely unlikely to break >> anything. > And by making basic asms use/clobber memory in particular, it means > code using them is less likely to break as the optimizers continue to > get smarter about memory loads/stores. > > I haven't gone through the actual patch yet, but I like it's basic goals. Other than the doc changes I already mentioned, I have no technical issues with the patch (although I don't speak 'internals' well enough to approve it either). That said, I'd like to make one final pitch for the alternate solution proposed by Richard Henderson: "deprecate and later completely remove basic asm within functions" IOW: Basic asm can only be used at "top level." Within functions, extended asm must be used. The reason I asked for clarification on what problem this patch is intended to solve, is that basic asm has a number of them. Given it: - Doesn't clobber memory - Doesn't clobber used registers - Doesn't clobber flags - Doesn't have dependencies This makes gcc's basic asm: - incompatible with user expectations. - incompatible with other compilers. - difficult for optimizers to correctly and consistently position. Bernd's patch adding the memory clobber does help with all of these. And I agree that it is unlikely to cause the generation of incorrect code. However unlike Andrew, I'm not prepared to say it doesn't 'hurt.' At a minimum, suddenly forcing an unexpected/unneeded memory clobber can adversely impact the optimization of surrounding code. This can be particularly annoying if the reason for the asm was to improve performance. And adding a memory clobber does add a dependency of sorts, which might cause the location of the asm to shift in an unfortunate way. And there's always the long-shot possibility that some weird quirk or (very) badly-written code will cause the asm to flat out fail when used with a memory clobber. And if this change does produce any of these problems, I feel pity for whoever has to track it down. But the real problem I have with this patch is that it doesn't actually solve the problem of user expectations. Or compatibility with other compilers. Or the problem with positioning. In fact this change makes the problem with expectations worse for the people who are knowledgeable about the current decades-old behavior. Ultimately, the design of gcc makes these problems inherently unsolvable. That's why I believe deprecation/removal is the real solution here. I realize deprecation/removal is drastic. Especially since basic asm (mostly) works as is. But fixing memory clobbers while leaving the rest broken feels like half a solution, meaning that some day we're going to have to fiddle with this again. If we are going to spend the time and effort (both ours and our users') to address basic-asm-in-a-function's problems, let's make sure we really solve them. dw
On 05/20/2016 07:50 AM, David Wohlferd wrote: > At a minimum, suddenly forcing an unexpected/unneeded memory clobber > can adversely impact the optimization of surrounding code. This can > be particularly annoying if the reason for the asm was to improve > performance. And adding a memory clobber does add a dependency of > sorts, which might cause the location of the asm to shift in an > unfortunate way. And there's always the long-shot possibility that > some weird quirk or (very) badly-written code will cause the asm to > flat out fail when used with a memory clobber. And if this change > does produce any of these problems, I feel pity for whoever has to > track it down. OTOH, if a memory clobber does change code gen it probably changes it in a way which better fits user expectations, and perhaps it fixes a bug. That's a win, and it is far, far more important than any other consideration. Given that a basic asm statements has neither inputs nor outputs, it must have side effects to be useful. All this patch does is recognize that fact. I'm not saying your scenario won't occur, but it won't in the majority of cases. > I realize deprecation/removal is drastic. Especially since basic > asm (mostly) works as is. But fixing memory clobbers while leaving > the rest broken feels like half a solution, meaning that some day > we're going to have to fiddle with this again. Yes, we will undoubtedly have to fiddle with basic asm again. We should plan for deprecation. But I think you're close to the all-or-nothing fallacy: that because this patch doesn't solve all the problems with basic asm, it isn't worth having. Andrew.
On Sun, 22 May 2016, Andrew Haley wrote: > On 05/20/2016 07:50 AM, David Wohlferd wrote: > > > At a minimum, suddenly forcing an unexpected/unneeded memory clobber > > can adversely impact the optimization of surrounding code. This can > > be particularly annoying if the reason for the asm was to improve > > performance. And adding a memory clobber does add a dependency of > > sorts, which might cause the location of the asm to shift in an > > unfortunate way. And there's always the long-shot possibility that > > some weird quirk or (very) badly-written code will cause the asm to > > flat out fail when used with a memory clobber. And if this change > > does produce any of these problems, I feel pity for whoever has to > > track it down. > > OTOH, if a memory clobber does change code gen it probably changes it > in a way which better fits user expectations, and perhaps it fixes a > bug. That's a win, and it is far, far more important than any other > consideration. > > Given that a basic asm statements has neither inputs nor outputs, it > must have side effects to be useful. All this patch does is recognize > that fact. I'm not saying your scenario won't occur, but it won't in > the majority of cases. > > > I realize deprecation/removal is drastic. Especially since basic > > asm (mostly) works as is. But fixing memory clobbers while leaving > > the rest broken feels like half a solution, meaning that some day > > we're going to have to fiddle with this again. > > Yes, we will undoubtedly have to fiddle with basic asm again. We > should plan for deprecation. > > But I think you're close to the all-or-nothing fallacy: that because > this patch doesn't solve all the problems with basic asm, it isn't > worth having. I think adding memory clobbers is worth having. I also think that deprecating basic asms would be a good thing, so can we please add a new warning for that? "warning: basic asms are deprecated" Richard.
On 05/22/2016 04:33 AM, Andrew Haley wrote: > On 05/20/2016 07:50 AM, David Wohlferd wrote: > >> At a minimum, suddenly forcing an unexpected/unneeded memory clobber >> can adversely impact the optimization of surrounding code. This can >> be particularly annoying if the reason for the asm was to improve >> performance. And adding a memory clobber does add a dependency of >> sorts, which might cause the location of the asm to shift in an >> unfortunate way. And there's always the long-shot possibility that >> some weird quirk or (very) badly-written code will cause the asm to >> flat out fail when used with a memory clobber. And if this change >> does produce any of these problems, I feel pity for whoever has to >> track it down. > > OTOH, if a memory clobber does change code gen it probably changes it > in a way which better fits user expectations, and perhaps it fixes a > bug. That's a win, and it is far, far more important than any other > consideration. My thoughts precisely. > >> I realize deprecation/removal is drastic. Especially since basic >> asm (mostly) works as is. But fixing memory clobbers while leaving >> the rest broken feels like half a solution, meaning that some day >> we're going to have to fiddle with this again. > > Yes, we will undoubtedly have to fiddle with basic asm again. We > should plan for deprecation. Right. There are some fundamental problems with basic asms and I think we want to deprecate them in the long term. In the immediate/medium term, I think addressing the memory dependency issue is the right thing to do. While it may make some code somewhere less optimized, it brings the basic asm semantics closer to what most programmers expect and prevents them from suddenly breaking as the optimizers continue to improve. If someone wants better optimized code, they ought to be using extended asms anyway. Jeff
On 5/23/2016 12:46 AM, Richard Biener wrote: > On Sun, 22 May 2016, Andrew Haley wrote: >> On 05/20/2016 07:50 AM, David Wohlferd wrote: >>> I realize deprecation/removal is drastic. Especially since basic >>> asm (mostly) works as is. But fixing memory clobbers while leaving >>> the rest broken feels like half a solution, meaning that some day >>> we're going to have to fiddle with this again. >> >> Yes, we will undoubtedly have to fiddle with basic asm again. We >> should plan for deprecation. > > I think adding memory clobbers is worth having. I also think that > deprecating basic asms would be a good thing, so can we please > add a new warning for that? "warning: basic asms are deprecated" I've still got the -Wbasic-asm patch where I proposed this for v6. I can dust it off again and re-submit it. A couple questions first: 1) In this patch the warning was disabled by default. But it sounds like you want it enabled by default? Easy to change, I'm just confirming your intent. 2) Is 'deprecated' handled differently than other types of warnings? There is a -Wno-deprecated, but it seems to have a very specific meaning that does not apply here. 3) The warning text in the old patch was "asm statement in function does not use extended syntax". The intent was: a) Don't make it sound like basic asm is completely gone, since it can still be used at top level. b) Don't make it sound like all inline asm is gone, since extended asm can still be used in functions. c) Convey all that in as few words as possible. Now that we want to add the word 'deprecated,' perhaps one of these: - Basic asm in functions is deprecated in favor of extended syntax - asm in functions without extended syntax is deprecated - Deprecated: basic asm in function - Deprecated: asm in function without extended syntax I like the last one (people may not know what 'basic' means in this context), but any of these would work for me. Preferences? In order to avoid conflicts, I'll wait for Bernd to commit his patch first. dw
Index: gcc/cfgexpand.c =================================================================== --- gcc/cfgexpand.c (revision 231412) +++ gcc/cfgexpand.c (working copy) @@ -2655,9 +2655,6 @@ expand_asm_loc (tree string, int vol, location_t l { rtx body; - if (TREE_CODE (string) == ADDR_EXPR) - string = TREE_OPERAND (string, 0); - body = gen_rtx_ASM_INPUT_loc (VOIDmode, ggc_strdup (TREE_STRING_POINTER (string)), locus); @@ -2664,6 +2661,34 @@ expand_asm_loc (tree string, int vol, location_t l MEM_VOLATILE_P (body) = vol; + /* Non-empty basic ASM implicitly clobbers memory. */ + if (TREE_STRING_LENGTH (string) != 0) + { + rtx asm_op, clob; + unsigned i, nclobbers; + auto_vec<rtx> input_rvec, output_rvec; + auto_vec<const char *> constraints; + auto_vec<rtx> clobber_rvec; + HARD_REG_SET clobbered_regs; + CLEAR_HARD_REG_SET (clobbered_regs); + + clob = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (VOIDmode)); + clobber_rvec.safe_push (clob); + + if (targetm.md_asm_adjust) + targetm.md_asm_adjust (output_rvec, input_rvec, + constraints, clobber_rvec, + clobbered_regs); + + asm_op = body; + nclobbers = clobber_rvec.length (); + body = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (1 + nclobbers)); + + XVECEXP (body, 0, 0) = asm_op; + for (i = 0; i < nclobbers; i++) + XVECEXP (body, 0, i + 1) = gen_rtx_CLOBBER (VOIDmode, clobber_rvec[i]); + } + emit_insn (body); } Index: gcc/compare-elim.c =================================================================== --- gcc/compare-elim.c (revision 231412) +++ gcc/compare-elim.c (working copy) @@ -162,7 +162,7 @@ arithmetic_flags_clobber_p (rtx_insn *insn) if (!NONJUMP_INSN_P (insn)) return false; pat = PATTERN (insn); - if (extract_asm_operands (pat)) + if (asm_noperands (pat) >= 0) return false; if (GET_CODE (pat) == PARALLEL && XVECLEN (pat, 0) == 2) Index: gcc/doc/extend.texi =================================================================== --- gcc/doc/extend.texi (revision 231412) +++ gcc/doc/extend.texi (working copy) @@ -7442,6 +7442,10 @@ all basic @code{asm} blocks use the assembler dial Basic @code{asm} provides no mechanism to provide different assembler strings for different dialects. +For basic @code{asm} with non-empty assembler string GCC assumes +the assembler block does not change any general purpose registers, +but it may read or write any globally accessible variable. + Here is an example of basic @code{asm} for i386: @example Index: gcc/final.c =================================================================== --- gcc/final.c (revision 231412) +++ gcc/final.c (working copy) @@ -2565,6 +2565,10 @@ final_scan_insn (rtx_insn *insn, FILE *file, int o (*debug_hooks->source_line) (last_linenum, last_filename, last_discriminator, is_stmt); + if (GET_CODE (body) == PARALLEL + && GET_CODE (XVECEXP (body, 0, 0)) == ASM_INPUT) + body = XVECEXP (body, 0, 0); + if (GET_CODE (body) == ASM_INPUT) { const char *string = XSTR (body, 0); Index: gcc/gimple.c =================================================================== --- gcc/gimple.c (revision 231412) +++ gcc/gimple.c (working copy) @@ -2567,6 +2567,10 @@ gimple_asm_clobbers_memory_p (const gasm *stmt) return true; } + /* Non-empty basic ASM implicitly clobbers memory. */ + if (gimple_asm_input_p (stmt) && strlen (gimple_asm_string (stmt)) != 0) + return true; + return false; } Index: gcc/ira.c =================================================================== --- gcc/ira.c (revision 231412) +++ gcc/ira.c (working copy) @@ -2229,7 +2229,7 @@ compute_regs_asm_clobbered (void) { df_ref def; - if (NONDEBUG_INSN_P (insn) && extract_asm_operands (PATTERN (insn))) + if (NONDEBUG_INSN_P (insn) && asm_noperands (PATTERN (insn)) >= 0) FOR_EACH_INSN_DEF (def, insn) { unsigned int dregno = DF_REF_REGNO (def); Index: gcc/recog.c =================================================================== --- gcc/recog.c (revision 231412) +++ gcc/recog.c (working copy) @@ -1470,6 +1470,8 @@ extract_asm_operands (rtx body) /* If BODY is an insn body that uses ASM_OPERANDS, return the number of operands (both input and output) in the insn. + If BODY is an insn body that uses ASM_INPUT with CLOBBERS in PARALLEL, + return 0. Otherwise return -1. */ int @@ -1476,16 +1478,26 @@ int asm_noperands (const_rtx body) { rtx asm_op = extract_asm_operands (CONST_CAST_RTX (body)); - int n_sets = 0; + int i, n_sets = 0; if (asm_op == NULL) - return -1; + { + if (GET_CODE (body) == PARALLEL && XVECLEN (body, 0) >= 2 + && GET_CODE (XVECEXP (body, 0, 0)) == ASM_INPUT) + { + /* body is [(asm_input ...) (clobber (reg ...))...]. */ + for (i = XVECLEN (body, 0) - 1; i > 0; i--) + if (GET_CODE (XVECEXP (body, 0, i)) != CLOBBER) + return -1; + return 0; + } + return -1; + } if (GET_CODE (body) == SET) n_sets = 1; else if (GET_CODE (body) == PARALLEL) { - int i; if (GET_CODE (XVECEXP (body, 0, 0)) == SET) { /* Multiple output operands, or 1 output plus some clobbers: @@ -1540,9 +1552,12 @@ asm_noperands (const_rtx body) the locations of the operands within the insn into the vector OPERAND_LOCS, and the constraints for the operands into CONSTRAINTS. Write the modes of the operands into MODES. + Write the location info into LOC. Return the assembler-template. + If BODY is an insn body that uses ASM_INPUT with CLOBBERS in PARALLEL, + return the basic assembly string. - If MODES, OPERAND_LOCS, CONSTRAINTS or OPERANDS is 0, + If LOC, MODES, OPERAND_LOCS, CONSTRAINTS or OPERANDS is 0, we don't store that info. */ const char * @@ -1603,6 +1618,12 @@ decode_asm_operands (rtx body, rtx *operands, rtx } nbase = i; } + else if (GET_CODE (asmop) == ASM_INPUT) + { + if (loc) + *loc = ASM_INPUT_SOURCE_LOCATION (asmop); + return XSTR (asmop, 0); + } break; } @@ -2244,7 +2265,8 @@ extract_insn (rtx_insn *insn) case PARALLEL: if ((GET_CODE (XVECEXP (body, 0, 0)) == SET && GET_CODE (SET_SRC (XVECEXP (body, 0, 0))) == ASM_OPERANDS) - || GET_CODE (XVECEXP (body, 0, 0)) == ASM_OPERANDS) + || GET_CODE (XVECEXP (body, 0, 0)) == ASM_OPERANDS + || GET_CODE (XVECEXP (body, 0, 0)) == ASM_INPUT) goto asm_insn; else goto normal_insn; Index: gcc/testsuite/gcc.target/i386/pr24414.c =================================================================== --- gcc/testsuite/gcc.target/i386/pr24414.c (revision 0) +++ gcc/testsuite/gcc.target/i386/pr24414.c (working copy) @@ -0,0 +1,13 @@ +/* { dg-do run } */ +/* { dg-options "-O2" } */ +int test; + +int +main () +{ + int x = test; + asm ("movl $1,test"); + if (x + test != 1) + __builtin_trap (); + return 0; +} Index: gcc/config/ia64/ia64.c =================================================================== --- gcc/config/ia64/ia64.c (revision 231412) +++ gcc/config/ia64/ia64.c (working copy) @@ -6549,6 +6549,7 @@ rtx_needs_barrier (rtx x, struct reg_flags flags, case USE: case CALL: case ASM_OPERANDS: + case ASM_INPUT: need_barrier |= rtx_needs_barrier (pat, flags, pred); break; Index: gcc/config/pa/pa.c =================================================================== --- gcc/config/pa/pa.c (revision 231412) +++ gcc/config/pa/pa.c (working copy) @@ -6399,7 +6399,7 @@ branch_to_delay_slot_p (rtx_insn *insn) the branch is followed by an asm. */ if (!insn || GET_CODE (PATTERN (insn)) == ASM_INPUT - || extract_asm_operands (PATTERN (insn)) != NULL_RTX + || asm_noperands (PATTERN (insn)) >= 0 || get_attr_length (insn) > 0) break; } @@ -6430,7 +6430,7 @@ branch_needs_nop_p (rtx_insn *insn) return TRUE; if (!(GET_CODE (PATTERN (insn)) == ASM_INPUT - || extract_asm_operands (PATTERN (insn)) != NULL_RTX) + || asm_noperands (PATTERN (insn)) >= 0) && get_attr_length (insn) > 0) break; } @@ -6454,7 +6454,7 @@ use_skip_p (rtx_insn *insn) /* We can't rely on the length of asms, so we can't skip asms. */ if (!insn || GET_CODE (PATTERN (insn)) == ASM_INPUT - || extract_asm_operands (PATTERN (insn)) != NULL_RTX) + || asm_noperands (PATTERN (insn)) >= 0) break; if (get_attr_length (insn) == 4 && jump_insn == next_active_insn (insn))