[v2,1/3] Allow memory operands for PTWRITE

Message ID 20181116035704.14820-2-andi@firstfloor.org
State New
Headers show
Series
  • [v2,1/3] Allow memory operands for PTWRITE
Related show

Commit Message

Andi Kleen Nov. 16, 2018, 3:57 a.m.
From: Andi Kleen <ak@linux.intel.com>


The earlier PTWRITE builtin definition was unnecessarily restrictive,
only allowing register input to PTWRITE. The instruction actually
supports memory operands too, so allow that too.

gcc/:

2018-11-15  Andi Kleen  <ak@linux.intel.com>

	* config/i386/i386.md: Allow memory operands to ptwrite.
---
 gcc/config/i386/i386.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.19.1

Comments

Uros Bizjak Nov. 16, 2018, 7:07 a.m. | #1
On Fri, Nov 16, 2018 at 4:57 AM Andi Kleen <andi@firstfloor.org> wrote:
>

> From: Andi Kleen <ak@linux.intel.com>

>

> The earlier PTWRITE builtin definition was unnecessarily restrictive,

> only allowing register input to PTWRITE. The instruction actually

> supports memory operands too, so allow that too.

>

> gcc/:

>

> 2018-11-15  Andi Kleen  <ak@linux.intel.com>

>

>         * config/i386/i386.md: Allow memory operands to ptwrite.


OK.

Thanks,
Uros.

> ---

>  gcc/config/i386/i386.md | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md

> index 44db8ac954c..9c359c0ca04 100644

> --- a/gcc/config/i386/i386.md

> +++ b/gcc/config/i386/i386.md

> @@ -19501,7 +19501,7 @@

>     (set_attr "prefix_extra" "2")])

>

>  (define_insn "ptwrite<mode>"

> -  [(unspec_volatile [(match_operand:SWI48 0 "register_operand" "r")]

> +  [(unspec_volatile [(match_operand:SWI48 0 "nonimmediate_operand" "rm")]

>                     UNSPECV_PTWRITE)]

>    "TARGET_PTWRITE"

>    "ptwrite\t%0"

> --

> 2.19.1

>
Richard Biener Nov. 20, 2018, 10:53 a.m. | #2
On Fri, Nov 16, 2018 at 8:07 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>

> On Fri, Nov 16, 2018 at 4:57 AM Andi Kleen <andi@firstfloor.org> wrote:

> >

> > From: Andi Kleen <ak@linux.intel.com>

> >

> > The earlier PTWRITE builtin definition was unnecessarily restrictive,

> > only allowing register input to PTWRITE. The instruction actually

> > supports memory operands too, so allow that too.

> >

> > gcc/:

> >

> > 2018-11-15  Andi Kleen  <ak@linux.intel.com>

> >

> >         * config/i386/i386.md: Allow memory operands to ptwrite.

>

> OK.


Btw, I wonder why the ptwrite builtin is in SPECIAL_ARGS2
commented as /* Add all special builtins with variable number of operands. */?

On the GIMPLE level this builtin also has quite some (bad) effects on
alias analysis and any related optimization (vectorization, etc.).  I'll have
to see where the instrumenting pass now resides.

Richard.

> Thanks,

> Uros.

>

> > ---

> >  gcc/config/i386/i386.md | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> >

> > diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md

> > index 44db8ac954c..9c359c0ca04 100644

> > --- a/gcc/config/i386/i386.md

> > +++ b/gcc/config/i386/i386.md

> > @@ -19501,7 +19501,7 @@

> >     (set_attr "prefix_extra" "2")])

> >

> >  (define_insn "ptwrite<mode>"

> > -  [(unspec_volatile [(match_operand:SWI48 0 "register_operand" "r")]

> > +  [(unspec_volatile [(match_operand:SWI48 0 "nonimmediate_operand" "rm")]

> >                     UNSPECV_PTWRITE)]

> >    "TARGET_PTWRITE"

> >    "ptwrite\t%0"

> > --

> > 2.19.1

> >
Andi Kleen Nov. 20, 2018, 6:36 p.m. | #3
On Tue, Nov 20, 2018 at 11:53:15AM +0100, Richard Biener wrote:
> On Fri, Nov 16, 2018 at 8:07 AM Uros Bizjak <ubizjak@gmail.com> wrote:

> >

> > On Fri, Nov 16, 2018 at 4:57 AM Andi Kleen <andi@firstfloor.org> wrote:

> > >

> > > From: Andi Kleen <ak@linux.intel.com>

> > >

> > > The earlier PTWRITE builtin definition was unnecessarily restrictive,

> > > only allowing register input to PTWRITE. The instruction actually

> > > supports memory operands too, so allow that too.

> > >

> > > gcc/:

> > >

> > > 2018-11-15  Andi Kleen  <ak@linux.intel.com>

> > >

> > >         * config/i386/i386.md: Allow memory operands to ptwrite.

> >

> > OK.

> 

> Btw, I wonder why the ptwrite builtin is in SPECIAL_ARGS2

> commented as /* Add all special builtins with variable number of operands. */?


i think i put it in the same place as a similar builtin. AFAIK
those others don't have variable arguments either, so the comment
may be wrong?
> 

> On the GIMPLE level this builtin also has quite some (bad) effects on

> alias analysis and any related optimization (vectorization, etc.).  I'll have

> to see where the instrumenting pass now resides.


It's fairly late now.

Any suggestions for improvements? At some point I removed the edges
like the old MPX builtins to minimize memory usage, but that was
removed during an earlier review cycle.

-Andi
Richard Biener Nov. 21, 2018, 2:47 p.m. | #4
On Tue, Nov 20, 2018 at 7:36 PM Andi Kleen <andi@firstfloor.org> wrote:
>

> On Tue, Nov 20, 2018 at 11:53:15AM +0100, Richard Biener wrote:

> > On Fri, Nov 16, 2018 at 8:07 AM Uros Bizjak <ubizjak@gmail.com> wrote:

> > >

> > > On Fri, Nov 16, 2018 at 4:57 AM Andi Kleen <andi@firstfloor.org> wrote:

> > > >

> > > > From: Andi Kleen <ak@linux.intel.com>

> > > >

> > > > The earlier PTWRITE builtin definition was unnecessarily restrictive,

> > > > only allowing register input to PTWRITE. The instruction actually

> > > > supports memory operands too, so allow that too.

> > > >

> > > > gcc/:

> > > >

> > > > 2018-11-15  Andi Kleen  <ak@linux.intel.com>

> > > >

> > > >         * config/i386/i386.md: Allow memory operands to ptwrite.

> > >

> > > OK.

> >

> > Btw, I wonder why the ptwrite builtin is in SPECIAL_ARGS2

> > commented as /* Add all special builtins with variable number of operands. */?

>

> i think i put it in the same place as a similar builtin. AFAIK

> those others don't have variable arguments either, so the comment

> may be wrong?


No idea...

> >

> > On the GIMPLE level this builtin also has quite some (bad) effects on

> > alias analysis and any related optimization (vectorization, etc.).  I'll have

> > to see where the instrumenting pass now resides.

>

> It's fairly late now.


OK, saw that.

> Any suggestions for improvements? At some point I removed the edges

> like the old MPX builtins to minimize memory usage, but that was

> removed during an earlier review cycle.


I guess it's fine now - it will have an effect on TER, limiting its ability
a bit, but otherwise the builtin only lives up to RTL expansion where
it becomes the UNSPEC_VOLATILE.  As said, instrumenting on
RTL would be an improvement, I think HJ might be able to help with that.

Richard.

> -Andi
H.J. Lu Nov. 21, 2018, 3:37 p.m. | #5
On Wed, Nov 21, 2018 at 6:48 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>

> On Tue, Nov 20, 2018 at 7:36 PM Andi Kleen <andi@firstfloor.org> wrote:

> >

> > On Tue, Nov 20, 2018 at 11:53:15AM +0100, Richard Biener wrote:

> > > On Fri, Nov 16, 2018 at 8:07 AM Uros Bizjak <ubizjak@gmail.com> wrote:

> > > >

> > > > On Fri, Nov 16, 2018 at 4:57 AM Andi Kleen <andi@firstfloor.org> wrote:

> > > > >

> > > > > From: Andi Kleen <ak@linux.intel.com>

> > > > >

> > > > > The earlier PTWRITE builtin definition was unnecessarily restrictive,

> > > > > only allowing register input to PTWRITE. The instruction actually

> > > > > supports memory operands too, so allow that too.

> > > > >

> > > > > gcc/:

> > > > >

> > > > > 2018-11-15  Andi Kleen  <ak@linux.intel.com>

> > > > >

> > > > >         * config/i386/i386.md: Allow memory operands to ptwrite.

> > > >

> > > > OK.

> > >

> > > Btw, I wonder why the ptwrite builtin is in SPECIAL_ARGS2

> > > commented as /* Add all special builtins with variable number of operands. */?

> >

> > i think i put it in the same place as a similar builtin. AFAIK

> > those others don't have variable arguments either, so the comment

> > may be wrong?

>

> No idea...

>

> > >

> > > On the GIMPLE level this builtin also has quite some (bad) effects on

> > > alias analysis and any related optimization (vectorization, etc.).  I'll have

> > > to see where the instrumenting pass now resides.

> >

> > It's fairly late now.

>

> OK, saw that.

>

> > Any suggestions for improvements? At some point I removed the edges

> > like the old MPX builtins to minimize memory usage, but that was

> > removed during an earlier review cycle.

>

> I guess it's fine now - it will have an effect on TER, limiting its ability

> a bit, but otherwise the builtin only lives up to RTL expansion where

> it becomes the UNSPEC_VOLATILE.  As said, instrumenting on

> RTL would be an improvement, I think HJ might be able to help with that.

>


What are the issues?

-- 
H.J.
Richard Biener Nov. 22, 2018, 1:24 p.m. | #6
On Wed, Nov 21, 2018 at 4:38 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>

> On Wed, Nov 21, 2018 at 6:48 AM Richard Biener

> <richard.guenther@gmail.com> wrote:

> >

> > On Tue, Nov 20, 2018 at 7:36 PM Andi Kleen <andi@firstfloor.org> wrote:

> > >

> > > On Tue, Nov 20, 2018 at 11:53:15AM +0100, Richard Biener wrote:

> > > > On Fri, Nov 16, 2018 at 8:07 AM Uros Bizjak <ubizjak@gmail.com> wrote:

> > > > >

> > > > > On Fri, Nov 16, 2018 at 4:57 AM Andi Kleen <andi@firstfloor.org> wrote:

> > > > > >

> > > > > > From: Andi Kleen <ak@linux.intel.com>

> > > > > >

> > > > > > The earlier PTWRITE builtin definition was unnecessarily restrictive,

> > > > > > only allowing register input to PTWRITE. The instruction actually

> > > > > > supports memory operands too, so allow that too.

> > > > > >

> > > > > > gcc/:

> > > > > >

> > > > > > 2018-11-15  Andi Kleen  <ak@linux.intel.com>

> > > > > >

> > > > > >         * config/i386/i386.md: Allow memory operands to ptwrite.

> > > > >

> > > > > OK.

> > > >

> > > > Btw, I wonder why the ptwrite builtin is in SPECIAL_ARGS2

> > > > commented as /* Add all special builtins with variable number of operands. */?

> > >

> > > i think i put it in the same place as a similar builtin. AFAIK

> > > those others don't have variable arguments either, so the comment

> > > may be wrong?

> >

> > No idea...

> >

> > > >

> > > > On the GIMPLE level this builtin also has quite some (bad) effects on

> > > > alias analysis and any related optimization (vectorization, etc.).  I'll have

> > > > to see where the instrumenting pass now resides.

> > >

> > > It's fairly late now.

> >

> > OK, saw that.

> >

> > > Any suggestions for improvements? At some point I removed the edges

> > > like the old MPX builtins to minimize memory usage, but that was

> > > removed during an earlier review cycle.

> >

> > I guess it's fine now - it will have an effect on TER, limiting its ability

> > a bit, but otherwise the builtin only lives up to RTL expansion where

> > it becomes the UNSPEC_VOLATILE.  As said, instrumenting on

> > RTL would be an improvement, I think HJ might be able to help with that.

> >

>

> What are the issues?


AFAIU inserting ptwrite only for values where the location allows a
variable to be infered from debug location lists _and_ properly
extend the location range to cover the ptwrite instruction itself  if
the value dies otherwise (like for stores).

See the thread about the instrumentation pass which currently
is implemented on GIMPLE.

Richard.

>

> --

> H.J.
H.J. Lu Nov. 22, 2018, 2:35 p.m. | #7
On Thu, Nov 22, 2018 at 5:24 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>

> On Wed, Nov 21, 2018 at 4:38 PM H.J. Lu <hjl.tools@gmail.com> wrote:

> >

> > On Wed, Nov 21, 2018 at 6:48 AM Richard Biener

> > <richard.guenther@gmail.com> wrote:

> > >

> > > On Tue, Nov 20, 2018 at 7:36 PM Andi Kleen <andi@firstfloor.org> wrote:

> > > >

> > > > On Tue, Nov 20, 2018 at 11:53:15AM +0100, Richard Biener wrote:

> > > > > On Fri, Nov 16, 2018 at 8:07 AM Uros Bizjak <ubizjak@gmail.com> wrote:

> > > > > >

> > > > > > On Fri, Nov 16, 2018 at 4:57 AM Andi Kleen <andi@firstfloor.org> wrote:

> > > > > > >

> > > > > > > From: Andi Kleen <ak@linux.intel.com>

> > > > > > >

> > > > > > > The earlier PTWRITE builtin definition was unnecessarily restrictive,

> > > > > > > only allowing register input to PTWRITE. The instruction actually

> > > > > > > supports memory operands too, so allow that too.

> > > > > > >

> > > > > > > gcc/:

> > > > > > >

> > > > > > > 2018-11-15  Andi Kleen  <ak@linux.intel.com>

> > > > > > >

> > > > > > >         * config/i386/i386.md: Allow memory operands to ptwrite.

> > > > > >

> > > > > > OK.

> > > > >

> > > > > Btw, I wonder why the ptwrite builtin is in SPECIAL_ARGS2

> > > > > commented as /* Add all special builtins with variable number of operands. */?

> > > >

> > > > i think i put it in the same place as a similar builtin. AFAIK

> > > > those others don't have variable arguments either, so the comment

> > > > may be wrong?

> > >

> > > No idea...

> > >

> > > > >

> > > > > On the GIMPLE level this builtin also has quite some (bad) effects on

> > > > > alias analysis and any related optimization (vectorization, etc.).  I'll have

> > > > > to see where the instrumenting pass now resides.

> > > >

> > > > It's fairly late now.

> > >

> > > OK, saw that.

> > >

> > > > Any suggestions for improvements? At some point I removed the edges

> > > > like the old MPX builtins to minimize memory usage, but that was

> > > > removed during an earlier review cycle.

> > >

> > > I guess it's fine now - it will have an effect on TER, limiting its ability

> > > a bit, but otherwise the builtin only lives up to RTL expansion where

> > > it becomes the UNSPEC_VOLATILE.  As said, instrumenting on

> > > RTL would be an improvement, I think HJ might be able to help with that.

> > >

> >

> > What are the issues?

>

> AFAIU inserting ptwrite only for values where the location allows a

> variable to be infered from debug location lists _and_ properly

> extend the location range to cover the ptwrite instruction itself  if

> the value dies otherwise (like for stores).

>

> See the thread about the instrumentation pass which currently

> is implemented on GIMPLE.


What are the issues for instrumenting as an RTL pass?


-- 
H.J.
Andi Kleen Nov. 22, 2018, 5:14 p.m. | #8
> > See the thread about the instrumentation pass which currently

> > is implemented on GIMPLE.

> 

> What are the issues for instrumenting as an RTL pass?


It just needs to be completely rewritten and might be more complicated.

And it might be more difficult to avoid redundant instrumentation
without SSA.

It might also be more fragile as RTL changes by target, although
right now it's only used on x86, so that wouldn't be a major concern.

-Andi

Patch

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 44db8ac954c..9c359c0ca04 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -19501,7 +19501,7 @@ 
    (set_attr "prefix_extra" "2")])
 
 (define_insn "ptwrite<mode>"
-  [(unspec_volatile [(match_operand:SWI48 0 "register_operand" "r")]
+  [(unspec_volatile [(match_operand:SWI48 0 "nonimmediate_operand" "rm")]
 		    UNSPECV_PTWRITE)]
   "TARGET_PTWRITE"
   "ptwrite\t%0"