| Message ID | 20181116035704.14820-2-andi@firstfloor.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] Allow memory operands for PTWRITE | expand |
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 >
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 > >
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
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
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?
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.
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?
> > 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
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"
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(-)