[PATCH] ira: volatile asm's are not moveable (PR82602)
A volatile asm statement can not be moved (relative to other volatile asm, etc.), but IRA could do it nevertheless. This patch fixes it. Testing on powerpc64-linux {-m32,-m64}; okay if it succeeds? Also for backports? Segher 2017-10-18 Segher Boessenkool PR rtl-optimization/82602 * ira.c (rtx_moveable_p): Return false for volatile asm. --- gcc/ira.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/gcc/ira.c b/gcc/ira.c index 046ce3b..8c93d3d 100644 --- a/gcc/ira.c +++ b/gcc/ira.c @@ -4400,6 +4400,12 @@ rtx_moveable_p (rtx *loc, enum op_type type) for a reason. */ return false; +case ASM_OPERANDS: + /* The same is true for volatile asm: it has unknown side effects, it + cannot be moved at will. */ + if (MEM_VOLATILE_P (x)) + return false; + default: break; } -- 1.8.3.1
Re: [PATCH] ira: volatile asm's are not moveable (PR82602)
Hi Segher, the patch looks ok for me. Just for my understanding: A memory clobber would also make rtx_moveable_p return false, thru the following case: case MEM: if (type == OP_IN && MEM_READONLY_P (x)) return rtx_moveable_p (&XEXP (x, 0), OP_IN); return false; ... case CLOBBER: return rtx_moveable_p (&SET_DEST (x), OP_OUT); because that memory clobber is in a parallel statement together with the ASM_OUTPUT. Right? Bernd.
Re: [PATCH] ira: volatile asm's are not moveable (PR82602)
On Wed, Oct 18, 2017 at 11:10:48AM +, Bernd Edlinger wrote: > A memory clobber would also make rtx_moveable_p return false, > thru the following case: > > case MEM: > if (type == OP_IN && MEM_READONLY_P (x)) > return rtx_moveable_p (&XEXP (x, 0), OP_IN); > return false; > > ... > > case CLOBBER: > return rtx_moveable_p (&SET_DEST (x), OP_OUT); > > > because that memory clobber is in a parallel statement > together with the ASM_OUTPUT. > > Right? Yes, that looks correct. And a memory clobber of course _should_ make the insn non-moveable as well -- it's an unknown side effect all by itself :-) Segher
Re: [PATCH] ira: volatile asm's are not moveable (PR82602)
On 10/18/17 13:30, Segher Boessenkool wrote: > On Wed, Oct 18, 2017 at 11:10:48AM +, Bernd Edlinger wrote: >> A memory clobber would also make rtx_moveable_p return false, >> thru the following case: >> >> case MEM: >>if (type == OP_IN && MEM_READONLY_P (x)) >> return rtx_moveable_p (&XEXP (x, 0), OP_IN); >>return false; >> >> ... >> >> case CLOBBER: >>return rtx_moveable_p (&SET_DEST (x), OP_OUT); >> >> >> because that memory clobber is in a parallel statement >> together with the ASM_OUTPUT. >> >> Right? > > Yes, that looks correct. And a memory clobber of course _should_ make > the insn non-moveable as well -- it's an unknown side effect all by > itself :-) > > Yeah, that's probably not what they wanted to hear, but supposed this is the only place where the volatility of ASM_OUTPUT is ignored, then adding a memory clobber to the asm stmt is the way to go if they want to play safe, especially when implementing spin-locks. Bernd.
Re: [PATCH] ira: volatile asm's are not moveable (PR82602)
On Wed, Oct 18, 2017 at 06:30:23AM -0500, Segher Boessenkool wrote: > On Wed, Oct 18, 2017 at 11:10:48AM +, Bernd Edlinger wrote: > > A memory clobber would also make rtx_moveable_p return false, > > thru the following case: > > > > case MEM: > > if (type == OP_IN && MEM_READONLY_P (x)) > > return rtx_moveable_p (&XEXP (x, 0), OP_IN); > > return false; > > > > ... > > > > case CLOBBER: > > return rtx_moveable_p (&SET_DEST (x), OP_OUT); > > > > > > because that memory clobber is in a parallel statement > > together with the ASM_OUTPUT. > > > > Right? > > Yes, that looks correct. And a memory clobber of course _should_ make > the insn non-moveable as well -- it's an unknown side effect all by > itself :-) Actually, it is not a side effect... Just kind of behaves like it. But it does *not* prevent the asm from being deleted, for example. Segher
Re: [PATCH] ira: volatile asm's are not moveable (PR82602)
On 10/18/17 15:46, Segher Boessenkool wrote: > On Wed, Oct 18, 2017 at 06:30:23AM -0500, Segher Boessenkool wrote: >> On Wed, Oct 18, 2017 at 11:10:48AM +, Bernd Edlinger wrote: >>> A memory clobber would also make rtx_moveable_p return false, >>> thru the following case: >>> >>> case MEM: >>>if (type == OP_IN && MEM_READONLY_P (x)) >>> return rtx_moveable_p (&XEXP (x, 0), OP_IN); >>>return false; >>> >>> ... >>> >>> case CLOBBER: >>>return rtx_moveable_p (&SET_DEST (x), OP_OUT); >>> >>> >>> because that memory clobber is in a parallel statement >>> together with the ASM_OUTPUT. >>> >>> Right? >> >> Yes, that looks correct. And a memory clobber of course _should_ make >> the insn non-moveable as well -- it's an unknown side effect all by >> itself :-) > > Actually, it is not a side effect... Just kind of behaves like it. > But it does *not* prevent the asm from being deleted, for example. > > Yes, the volatile is still necessary. Bernd.
Re: [PATCH] ira: volatile asm's are not moveable (PR82602)
On Wed, Oct 18, 2017 at 01:50:31PM +, Bernd Edlinger wrote: > Yes, the volatile is still necessary. Certainly. And to work around the bug, it should work to mention some hard register as asm input. Ideally something that is live anyway; perhaps the stack pointer :-) Like so: __asm volatile ("mrs %0,PRIMASK" : "=r" (status) :: "sp"); (I tested this, it does work around the bug). Or hey, why not the program counter, that should make it even clearer something is funny here: __asm volatile ("mrs %0,PRIMASK" : "=r" (status) :: "pc"); (also tested, works fine). Segher
Re: [PATCH] ira: volatile asm's are not moveable (PR82602)
Hi, On Wed, 18 Oct 2017, Segher Boessenkool wrote: > Certainly. And to work around the bug, it should work to mention some > hard register as asm input. Ideally something that is live anyway; > perhaps the stack pointer :-) Like so: > > __asm volatile ("mrs %0,PRIMASK" : "=r" (status) :: "sp"); > > (I tested this, it does work around the bug). > > Or hey, why not the program counter, that should make it even clearer > something is funny here: > > __asm volatile ("mrs %0,PRIMASK" : "=r" (status) :: "pc"); > > (also tested, works fine). Both of these are not asm inputs but clobbers, though :) And clobbering "pc" could have funny effects if any of our allocators ever would produce save/restore code around such asms for such clobbers. Ciao, Michael.
Re: [PATCH] ira: volatile asm's are not moveable (PR82602)
On Wed, Oct 18, 2017 at 05:17:19PM +0200, Michael Matz wrote: > On Wed, 18 Oct 2017, Segher Boessenkool wrote: > > > Certainly. And to work around the bug, it should work to mention some > > hard register as asm input. Ideally something that is live anyway; > > perhaps the stack pointer :-) Like so: > > > > __asm volatile ("mrs %0,PRIMASK" : "=r" (status) :: "sp"); > > > > (I tested this, it does work around the bug). > > > > Or hey, why not the program counter, that should make it even clearer > > something is funny here: > > > > __asm volatile ("mrs %0,PRIMASK" : "=r" (status) :: "pc"); > > > > (also tested, works fine). > > Both of these are not asm inputs but clobbers, though :) Yeah I typed (and tested) the code after writing that first sentence. Clobbering was easier :-) > And clobbering > "pc" could have funny effects if any of our allocators ever would produce > save/restore code around such asms for such clobbers. Yeah -- but sp (and pc, if it exists) is a fixed register, so we don't do such things :-) Segher
Re: [PATCH] ira: volatile asm's are not moveable (PR82602)
On 10/18/2017 06:48 AM, Segher Boessenkool wrote: A volatile asm statement can not be moved (relative to other volatile asm, etc.), but IRA could do it nevertheless. This patch fixes it. Testing on powerpc64-linux {-m32,-m64}; okay if it succeeds? Also for backports? Although I am not an author of this code, the patch looks ok for me. ASM_OPERANDS with volatile memory creates a barrier. I guess still a few such cases are missed for rtx_moveable_p. The reference for such cases should be function sched_deps.c::sched_analyze. You can commit it to the trunk and backport. It should be safe. Segher, thank you for working on the PR. 2017-10-18 Segher Boessenkool PR rtl-optimization/82602 * ira.c (rtx_moveable_p): Return false for volatile asm. --- gcc/ira.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/gcc/ira.c b/gcc/ira.c index 046ce3b..8c93d3d 100644 --- a/gcc/ira.c +++ b/gcc/ira.c @@ -4400,6 +4400,12 @@ rtx_moveable_p (rtx *loc, enum op_type type) for a reason. */ return false; +case ASM_OPERANDS: + /* The same is true for volatile asm: it has unknown side effects, it + cannot be moved at will. */ + if (MEM_VOLATILE_P (x)) + return false; + default: break; }