[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;
}
