Patch RFA: Add option -fcollectible-pointers, use it in ivopts

Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

As discussed at https://gcc.gnu.org/ml/gcc/2016-01/msg00023.html , the
Go frontend needs some way to prevent ivopts from temporarily removing
all pointers into a memory block.  This patch adds a new option
-fcollectible-pointers which makes that happen.  This is not the best
way to solve the problem, but it seems safe for GCC 6.

I made the option -fcollectible-pointers intending that any similar
optimizations (or, rather, non-optimizations) can be captured in the
same option.  And if we develop a better approach for ivopts, it can
still be covered under this option name.

Bootstrapped and tested on x86_64-pc-linux-gnu.  OK for mainline?

Ian

gcc/ChangeLog:

2016-01-19  Ian Lance Taylor  <iant@google.com>

        * common.opt (fcollectible-pointers): New option.
        * tree-ssa-loop-ivopts.c (add_autoinc_candidates): If
        -fcollectible-pointers, skip pointers.
        * doc/invoke.texi (Optimize Options): Document
        -fcollectible-pointers

gcc/testsuite/ChangeLog:

2016-01-19  Ian Lance Taylor  <iant@google.com>

        * gcc.dg/tree-ssa/ivopt_5.c: New test.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3092 bytes --]

Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 232580)
+++ gcc/common.opt	(working copy)
@@ -1025,6 +1025,10 @@
 Common Var(flag_checking) Init(CHECKING_P)
 Perform internal consistency checkings.
 
+fcollectible-pointers
+Common Report Var(flag_collectible_pointers) Optimization
+Ensure that pointers are always collectible by a garbage collector.
+
 fcombine-stack-adjustments
 Common Report Var(flag_combine_stack_adjustments) Optimization
 Looks for opportunities to reduce stack adjustments and stack references.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 232580)
+++ gcc/doc/invoke.texi	(working copy)
@@ -6621,6 +6621,17 @@
 If you use @option{-Wunsafe-loop-optimizations}, the compiler warns you
 if it finds this kind of loop.
 
+@item -fcollectible-pointers
+@opindex fcollectible-pointers
+This option tells the compiler that a garbage collector will be used,
+and that therefore the compiled code must retain a live pointer into
+all memory blocks.  The compiler is permitted to construct a pointer
+that is outside the bounds of a memory block, but it must ensure that
+given a pointer into memory, some pointer into that memory remains
+live in the compiled code whenever it is live in the source code.
+This option is disabled by default for most languages, enabled by
+default for languages that use garbage collection.
+
 @item -fcrossjumping
 @opindex fcrossjumping
 Perform cross-jumping transformation.
Index: gcc/testsuite/gcc.dg/tree-ssa/ivopt_5.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/ivopt_5.c	(revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/ivopt_5.c	(working copy)
@@ -0,0 +1,14 @@
+/* { dg-options "-O2 -fdump-tree-ivopts -fcollectible-pointers" } */
+
+/* No ivopts here when using -fcollectible-pointers.  */
+void foo (char *pstart, int n)
+{
+  char *p;
+  char *pend = pstart + n;
+
+  for (p = pstart; p < pend; p++)
+    *p = 1;
+}
+
+
+/* { dg-final { scan-tree-dump-times "ivtmp.\[0-9_\]* = PHI <" 0 "ivopts"} } */
Index: gcc/tree-ssa-loop-ivopts.c
===================================================================
--- gcc/tree-ssa-loop-ivopts.c	(revision 232580)
+++ gcc/tree-ssa-loop-ivopts.c	(working copy)
@@ -2956,6 +2956,17 @@
       || !cst_and_fits_in_hwi (step))
     return;
 
+  // -fcollectible-pointers means that we have to keep a real pointer
+  // live, but the ivopts code may replace a real pointer with one
+  // pointing before or after the memory block that is then adjusted
+  // into the memory block during the loop.
+  // FIXME: It would likely be better to actually force the pointer
+  // live and still use ivopts; for example, it would be enough to
+  // write the pointer into memory and keep it there until after the
+  // loop.
+  if (flag_collectible_pointers && POINTER_TYPE_P (TREE_TYPE (base)))
+    return;
+
   cstepi = int_cst_value (step);
 
   mem_mode = TYPE_MODE (TREE_TYPE (*use->op_p));

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Richard Biener]

On Wed, Jan 20, 2016 at 6:48 AM, Ian Lance Taylor <iant@golang.org> wrote:
> As discussed at https://gcc.gnu.org/ml/gcc/2016-01/msg00023.html , the
> Go frontend needs some way to prevent ivopts from temporarily removing
> all pointers into a memory block.  This patch adds a new option
> -fcollectible-pointers which makes that happen.  This is not the best
> way to solve the problem, but it seems safe for GCC 6.
>
> I made the option -fcollectible-pointers intending that any similar
> optimizations (or, rather, non-optimizations) can be captured in the
> same option.  And if we develop a better approach for ivopts, it can
> still be covered under this option name.
>
> Bootstrapped and tested on x86_64-pc-linux-gnu.  OK for mainline?

+  // -fcollectible-pointers means that we have to keep a real pointer
+  // live, but the ivopts code may replace a real pointer with one
+  // pointing before or after the memory block that is then adjusted
+  // into the memory block during the loop.
+  // FIXME: It would likely be better to actually force the pointer
+  // live and still use ivopts; for example, it would be enough to
+  // write the pointer into memory and keep it there until after the
+  // loop.
+  if (flag_collectible_pointers && POINTER_TYPE_P (TREE_TYPE (base)))
+    return;

please use C-style comments.  The above is to add_autoinc_candidates
which I find weird - we certainly produce out-of-bound pointers even on
x86 which isn't auto-inc.  So this can't be a complete fix.  I guess you
are correct in disabling some offsetted address IV candidates but
I think there's some other issues regarding to exit test replacement
maybe (replace ptr <= ptr2 with ptr != ptr2 or so).

While the docs of the option look fine I find

+fcollectible-pointers
+Common Report Var(flag_collectible_pointers) Optimization
+Ensure that pointers are always collectible by a garbage collector

somewhat confusing (we don't collect pointers but pointed-to memory).
Maybe "Ensure that derived pointers always point to the original object"?
In that light -fcollectible-pointers is a bad option name as well.  Maybe
-fall-pointers-are-gc-roots or sth like that.

Richard.

> Ian
>
> gcc/ChangeLog:
>
> 2016-01-19  Ian Lance Taylor  <iant@google.com>
>
>         * common.opt (fcollectible-pointers): New option.
>         * tree-ssa-loop-ivopts.c (add_autoinc_candidates): If
>         -fcollectible-pointers, skip pointers.
>         * doc/invoke.texi (Optimize Options): Document
>         -fcollectible-pointers
>
> gcc/testsuite/ChangeLog:
>
> 2016-01-19  Ian Lance Taylor  <iant@google.com>
>
>         * gcc.dg/tree-ssa/ivopt_5.c: New test.

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Ian Lance Taylor]

On Wed, Jan 20, 2016 at 3:13 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Jan 20, 2016 at 6:48 AM, Ian Lance Taylor <iant@golang.org> wrote:
>> As discussed at https://gcc.gnu.org/ml/gcc/2016-01/msg00023.html , the
>> Go frontend needs some way to prevent ivopts from temporarily removing
>> all pointers into a memory block.  This patch adds a new option
>> -fcollectible-pointers which makes that happen.  This is not the best
>> way to solve the problem, but it seems safe for GCC 6.
>>
>> I made the option -fcollectible-pointers intending that any similar
>> optimizations (or, rather, non-optimizations) can be captured in the
>> same option.  And if we develop a better approach for ivopts, it can
>> still be covered under this option name.
>>
>> Bootstrapped and tested on x86_64-pc-linux-gnu.  OK for mainline?
>
> +  // -fcollectible-pointers means that we have to keep a real pointer
> +  // live, but the ivopts code may replace a real pointer with one
> +  // pointing before or after the memory block that is then adjusted
> +  // into the memory block during the loop.
> +  // FIXME: It would likely be better to actually force the pointer
> +  // live and still use ivopts; for example, it would be enough to
> +  // write the pointer into memory and keep it there until after the
> +  // loop.
> +  if (flag_collectible_pointers && POINTER_TYPE_P (TREE_TYPE (base)))
> +    return;
>
> please use C-style comments.

Whoops, sorry, too much Go coding.

> The above is to add_autoinc_candidates
> which I find weird - we certainly produce out-of-bound pointers even on
> x86 which isn't auto-inc.

Despite the name, this is used on all systems.  That is the function
where we consider using BASE + STEP * i in  a loop.

> So this can't be a complete fix.  I guess you
> are correct in disabling some offsetted address IV candidates but
> I think there's some other issues regarding to exit test replacement
> maybe (replace ptr <= ptr2 with ptr != ptr2 or so).

I'll look into that.

> While the docs of the option look fine I find
>
> +fcollectible-pointers
> +Common Report Var(flag_collectible_pointers) Optimization
> +Ensure that pointers are always collectible by a garbage collector
>
> somewhat confusing (we don't collect pointers but pointed-to memory).
> Maybe "Ensure that derived pointers always point to the original object"?
> In that light -fcollectible-pointers is a bad option name as well.  Maybe
> -fall-pointers-are-gc-roots or sth like that.

I'm OK with that, or how about -fkeep-gc-roots-live?

Ian

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Richard Biener]

On Wed, Jan 20, 2016 at 3:02 PM, Ian Lance Taylor <iant@golang.org> wrote:
> On Wed, Jan 20, 2016 at 3:13 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jan 20, 2016 at 6:48 AM, Ian Lance Taylor <iant@golang.org> wrote:
>>> As discussed at https://gcc.gnu.org/ml/gcc/2016-01/msg00023.html , the
>>> Go frontend needs some way to prevent ivopts from temporarily removing
>>> all pointers into a memory block.  This patch adds a new option
>>> -fcollectible-pointers which makes that happen.  This is not the best
>>> way to solve the problem, but it seems safe for GCC 6.
>>>
>>> I made the option -fcollectible-pointers intending that any similar
>>> optimizations (or, rather, non-optimizations) can be captured in the
>>> same option.  And if we develop a better approach for ivopts, it can
>>> still be covered under this option name.
>>>
>>> Bootstrapped and tested on x86_64-pc-linux-gnu.  OK for mainline?
>>
>> +  // -fcollectible-pointers means that we have to keep a real pointer
>> +  // live, but the ivopts code may replace a real pointer with one
>> +  // pointing before or after the memory block that is then adjusted
>> +  // into the memory block during the loop.
>> +  // FIXME: It would likely be better to actually force the pointer
>> +  // live and still use ivopts; for example, it would be enough to
>> +  // write the pointer into memory and keep it there until after the
>> +  // loop.
>> +  if (flag_collectible_pointers && POINTER_TYPE_P (TREE_TYPE (base)))
>> +    return;
>>
>> please use C-style comments.
>
> Whoops, sorry, too much Go coding.
>
>> The above is to add_autoinc_candidates
>> which I find weird - we certainly produce out-of-bound pointers even on
>> x86 which isn't auto-inc.
>
> Despite the name, this is used on all systems.  That is the function
> where we consider using BASE + STEP * i in  a loop.
>
>> So this can't be a complete fix.  I guess you
>> are correct in disabling some offsetted address IV candidates but
>> I think there's some other issues regarding to exit test replacement
>> maybe (replace ptr <= ptr2 with ptr != ptr2 or so).
>
> I'll look into that.
>
>> While the docs of the option look fine I find
>>
>> +fcollectible-pointers
>> +Common Report Var(flag_collectible_pointers) Optimization
>> +Ensure that pointers are always collectible by a garbage collector
>>
>> somewhat confusing (we don't collect pointers but pointed-to memory).
>> Maybe "Ensure that derived pointers always point to the original object"?
>> In that light -fcollectible-pointers is a bad option name as well.  Maybe
>> -fall-pointers-are-gc-roots or sth like that.
>
> I'm OK with that, or how about -fkeep-gc-roots-live?

Sounds good.

Richard.

> Ian

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

Updated patch.

I've verified that I'm changing the only relevant place in
tree-ssa-loop-ivopts.c that creates a POINTER_PLUS_EXPR, so I do think
that this is the only changed to fix the problem for ivopts.

OK for mainline?

Ian

gcc/ChangeLog:

2016-01-22  Ian Lance Taylor  <iant@google.com>

* common.opt (fkeep-gc-roots-live): New option.
* tree-ssa-loop-ivopts.c (add_autoinc_candidates): If
-fkeep-gc-roots-live, skip pointers.
* doc/invoke.texi (Optimize Options): Document
-fkeep-gc-roots-live.

gcc/testsuite/ChangeLog:

2016-01-22  Ian Lance Taylor  <iant@google.com>

* gcc.dg/tree-ssa/ivopt_5.c: New test.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 3577 bytes --]

Index: common.opt
===================================================================
--- common.opt	(revision 232580)
+++ common.opt	(working copy)
@@ -1380,6 +1380,10 @@
 Enable hoisting adjacent loads to encourage generating conditional move
 instructions.
 
+fkeep-gc-roots-live
+Common Report Var(flag_keep_gc_roots_live) Optimization
+Always keep a pointer to a live memory block
+
 floop-parallelize-all
 Common Report Var(flag_loop_parallelize_all) Optimization
 Mark all loops as parallel.
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 232580)
+++ doc/invoke.texi	(working copy)
@@ -359,7 +359,7 @@
 -fno-ira-share-spill-slots @gol
 -fisolate-erroneous-paths-dereference -fisolate-erroneous-paths-attribute @gol
 -fivopts -fkeep-inline-functions -fkeep-static-functions @gol
--fkeep-static-consts -flive-range-shrinkage @gol
+-fkeep-static-consts -fkeep-gc-roots-live -flive-range-shrinkage @gol
 -floop-block -floop-interchange -floop-strip-mine @gol
 -floop-unroll-and-jam -floop-nest-optimize @gol
 -floop-parallelize-all -flra-remat -flto -flto-compression-level @gol
@@ -6621,6 +6621,17 @@
 If you use @option{-Wunsafe-loop-optimizations}, the compiler warns you
 if it finds this kind of loop.
 
+@item -fkeep-gc-roots-live
+@opindex fkeep-gc-roots-live
+This option tells the compiler that a garbage collector will be used,
+and that therefore the compiled code must retain a live pointer into
+all memory blocks.  The compiler is permitted to construct a pointer
+that is outside the bounds of a memory block, but it must ensure that
+given a pointer into memory, some pointer into that memory remains
+live in the compiled code whenever it is live in the source code.
+This option is disabled by default for most languages, enabled by
+default for languages that use garbage collection.
+
 @item -fcrossjumping
 @opindex fcrossjumping
 Perform cross-jumping transformation.
Index: testsuite/gcc.dg/tree-ssa/ivopt_5.c
===================================================================
--- testsuite/gcc.dg/tree-ssa/ivopt_5.c	(revision 0)
+++ testsuite/gcc.dg/tree-ssa/ivopt_5.c	(working copy)
@@ -0,0 +1,23 @@
+/* { dg-options "-O2 -fdump-tree-ivopts -fkeep-gc-roots-live" } */
+
+/* No ivopts here when using -fkeep-gc-roots-live.   */
+
+void foo (char *pstart, int n)
+{
+  char *p;
+  char *pend = pstart + n;
+
+  for (p = pstart; p < pend; p++)
+    *p = 1;
+}
+
+void foo1 (char *pstart, int n)
+{
+  char *p;
+  char *pend = pstart + n;
+
+  for (p = pstart; p != pend; p++)
+    *p = 1;
+}
+
+/* { dg-final { scan-tree-dump-times "ivtmp.\[0-9_\]* = PHI <" 0 "ivopts"} } */
Index: tree-ssa-loop-ivopts.c
===================================================================
--- tree-ssa-loop-ivopts.c	(revision 232580)
+++ tree-ssa-loop-ivopts.c	(working copy)
@@ -2956,6 +2956,16 @@
       || !cst_and_fits_in_hwi (step))
     return;
 
+  /* -fkeep-gc-roots-live means that we have to keep a real pointer
+     live, but the ivopts code may replace a real pointer with one
+     pointing before or after the memory block that is then adjusted
+     into the memory block during the loop.  FIXME: It would likely be
+     better to actually force the pointer live and still use ivopts;
+     for example, it would be enough to write the pointer into memory
+     and keep it there until after the loop.  */
+  if (flag_keep_gc_roots_live && POINTER_TYPE_P (TREE_TYPE (base)))
+    return;
+
   cstepi = int_cst_value (step);
 
   mem_mode = TYPE_MODE (TREE_TYPE (*use->op_p));

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Bernd Schmidt]

On 01/22/2016 08:03 PM, Ian Lance Taylor wrote:
> Updated patch.
>
> I've verified that I'm changing the only relevant place in
> tree-ssa-loop-ivopts.c that creates a POINTER_PLUS_EXPR, so I do think
> that this is the only changed to fix the problem for ivopts.

I don't think so. One of the problems with ivopts is that it likes to 
cast everything to unsigned int, so looking for POINTER_PLUS_EXPR 
wouldn't find all affected spots. At least this used to happen, I didn't 
check recently. Also, a lot of the generated expressions are built by 
tree-affine.c rather than in ivopts directly.


Bernd

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Ian Lance Taylor]

On Fri, Jan 22, 2016 at 11:25 AM, Bernd Schmidt <bernds_cb1@t-online.de> wrote:
> On 01/22/2016 08:03 PM, Ian Lance Taylor wrote:
>>
>> Updated patch.
>>
>> I've verified that I'm changing the only relevant place in
>> tree-ssa-loop-ivopts.c that creates a POINTER_PLUS_EXPR, so I do think
>> that this is the only changed to fix the problem for ivopts.
>
>
> I don't think so. One of the problems with ivopts is that it likes to cast
> everything to unsigned int, so looking for POINTER_PLUS_EXPR wouldn't find
> all affected spots. At least this used to happen, I didn't check recently.
> Also, a lot of the generated expressions are built by tree-affine.c rather
> than in ivopts directly.

Thanks for the tip.  I moved the check to add_candidate_1 instead.
This is before the point where it converts to an integer type.  This
approach is better anyhow, as it permits a pointer loop to use an
integer induction variable for the offset.  My tests still pass, as
does bootstrap/testsuite on x86_64-pc-linux-gnu.

Does this look OK for mainline?

Ian


gcc/ChangeLog:

2016-01-22  Ian Lance Taylor  <iant@google.com>

* common.opt (fkeep-gc-roots-live): New option.
* tree-ssa-loop-ivopts.c (add_candidate_1): If
-fkeep-gc-roots-live, skip pointers.
(add_iv_candidate_for_biv): Handle add_candidate_1 returning
NULL.
* doc/invoke.texi (Optimize Options): Document
-fkeep-gc-roots-live.

gcc/testsuite/ChangeLog:

2016-01-22  Ian Lance Taylor  <iant@google.com>

* gcc.dg/tree-ssa/ivopt_5.c: New test.

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Sandra Loosemore]

On 01/22/2016 12:03 PM, Ian Lance Taylor wrote:
> Index: common.opt
> ===================================================================
> --- common.opt	(revision 232580)
> +++ common.opt	(working copy)
> @@ -1380,6 +1380,10 @@
>  Enable hoisting adjacent loads to encourage generating conditional move
>  instructions.
>
> +fkeep-gc-roots-live
> +Common Report Var(flag_keep_gc_roots_live) Optimization
> +Always keep a pointer to a live memory block
> +

I think these option descriptions are supposed to end in a period.

>  floop-parallelize-all
>  Common Report Var(flag_loop_parallelize_all) Optimization
>  Mark all loops as parallel.
> Index: doc/invoke.texi
> ===================================================================
> --- doc/invoke.texi	(revision 232580)
> +++ doc/invoke.texi	(working copy)
> @@ -359,7 +359,7 @@
>  -fno-ira-share-spill-slots @gol
>  -fisolate-erroneous-paths-dereference -fisolate-erroneous-paths-attribute @gol
>  -fivopts -fkeep-inline-functions -fkeep-static-functions @gol
> --fkeep-static-consts -flive-range-shrinkage @gol
> +-fkeep-static-consts -fkeep-gc-roots-live -flive-range-shrinkage @gol

It looks like this list is supposed to be alphabetized, in which case 
the new one should be inserted before -fkeep-inline-functions, not after 
-fkeep-static-consts.

> @@ -6621,6 +6621,17 @@
>  If you use @option{-Wunsafe-loop-optimizations}, the compiler warns you
>  if it finds this kind of loop.
>
> +@item -fkeep-gc-roots-live
> +@opindex fkeep-gc-roots-live
> +This option tells the compiler that a garbage collector will be used,
> +and that therefore the compiled code must retain a live pointer into
> +all memory blocks.  The compiler is permitted to construct a pointer
> +that is outside the bounds of a memory block, but it must ensure that
> +given a pointer into memory, some pointer into that memory remains
> +live in the compiled code whenever it is live in the source code.
> +This option is disabled by default for most languages, enabled by
> +default for languages that use garbage collection.
> +

This is OK.

Someone else will have to review the code parts of the patch.

-Sandra

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Ian Lance Taylor]

On Sat, Jan 23, 2016 at 5:42 PM, Sandra Loosemore
<sandra@codesourcery.com> wrote:
>
> I think these option descriptions are supposed to end in a period.

Thanks for the doc comments; will fix before submitting if the overall
patch is approved.

Ian

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Bernd Schmidt]

On 01/23/2016 12:52 AM, Ian Lance Taylor wrote:

> 2016-01-22  Ian Lance Taylor  <iant@google.com>
>
> * common.opt (fkeep-gc-roots-live): New option.
> * tree-ssa-loop-ivopts.c (add_candidate_1): If
> -fkeep-gc-roots-live, skip pointers.
> (add_iv_candidate_for_biv): Handle add_candidate_1 returning
> NULL.
> * doc/invoke.texi (Optimize Options): Document
> -fkeep-gc-roots-live.
>
> gcc/testsuite/ChangeLog:
>
> 2016-01-22  Ian Lance Taylor  <iant@google.com>
>
> * gcc.dg/tree-ssa/ivopt_5.c: New test.

Patch not attached?


Bernd

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Ian Lance Taylor]

On Mon, Jan 25, 2016 at 3:39 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 01/23/2016 12:52 AM, Ian Lance Taylor wrote:
>
>> 2016-01-22  Ian Lance Taylor  <iant@google.com>
>>
>> * common.opt (fkeep-gc-roots-live): New option.
>> * tree-ssa-loop-ivopts.c (add_candidate_1): If
>> -fkeep-gc-roots-live, skip pointers.
>> (add_iv_candidate_for_biv): Handle add_candidate_1 returning
>> NULL.
>> * doc/invoke.texi (Optimize Options): Document
>> -fkeep-gc-roots-live.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2016-01-22  Ian Lance Taylor  <iant@google.com>
>>
>> * gcc.dg/tree-ssa/ivopt_5.c: New test.
>
>
> Patch not attached?

The patch is there in the mailing list.  See the attachment on
https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01781.html .

Ian

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Bernd Schmidt]

On 01/25/2016 05:03 PM, Ian Lance Taylor wrote:
> On Mon, Jan 25, 2016 at 3:39 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>> On 01/23/2016 12:52 AM, Ian Lance Taylor wrote:
>>
>>> 2016-01-22  Ian Lance Taylor  <iant@google.com>
>>>
>>> * common.opt (fkeep-gc-roots-live): New option.
>>> * tree-ssa-loop-ivopts.c (add_candidate_1): If
>>> -fkeep-gc-roots-live, skip pointers.
>>> (add_iv_candidate_for_biv): Handle add_candidate_1 returning
>>> NULL.
>>> * doc/invoke.texi (Optimize Options): Document
>>> -fkeep-gc-roots-live.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2016-01-22  Ian Lance Taylor  <iant@google.com>
>>>
>>> * gcc.dg/tree-ssa/ivopt_5.c: New test.
>>
>>
>> Patch not attached?
>
> The patch is there in the mailing list.  See the attachment on
> https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01781.html .

That seems to be the old patch. At least it doesn't seem to match the 
ChangeLog quoted above.


Bernd

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 986 bytes --]

On Tue, Jan 26, 2016 at 4:10 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>
>>> On 01/23/2016 12:52 AM, Ian Lance Taylor wrote:
>>>
>>>> 2016-01-22  Ian Lance Taylor  <iant@google.com>
>>>>
>>>> * common.opt (fkeep-gc-roots-live): New option.
>>>> * tree-ssa-loop-ivopts.c (add_candidate_1): If
>>>> -fkeep-gc-roots-live, skip pointers.
>>>> (add_iv_candidate_for_biv): Handle add_candidate_1 returning
>>>> NULL.
>>>> * doc/invoke.texi (Optimize Options): Document
>>>> -fkeep-gc-roots-live.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> 2016-01-22  Ian Lance Taylor  <iant@google.com>
>>>>
>>>> * gcc.dg/tree-ssa/ivopt_5.c: New test.
>>>
>>>
>>>
>>> Patch not attached?
>>
>>
>> The patch is there in the mailing list.  See the attachment on
>> https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01781.html .
>
>
> That seems to be the old patch. At least it doesn't seem to match the
> ChangeLog quoted above.

I'm sorry, you're quite right.  That's odd.  Here is the actual patch.

Ian

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 4137 bytes --]

Index: common.opt
===================================================================
--- common.opt	(revision 232580)
+++ common.opt	(working copy)
@@ -1380,6 +1380,10 @@
 Enable hoisting adjacent loads to encourage generating conditional move
 instructions.
 
+fkeep-gc-roots-live
+Common Report Var(flag_keep_gc_roots_live) Optimization
+Always keep a pointer to a live memory block
+
 floop-parallelize-all
 Common Report Var(flag_loop_parallelize_all) Optimization
 Mark all loops as parallel.
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 232580)
+++ doc/invoke.texi	(working copy)
@@ -359,7 +359,7 @@
 -fno-ira-share-spill-slots @gol
 -fisolate-erroneous-paths-dereference -fisolate-erroneous-paths-attribute @gol
 -fivopts -fkeep-inline-functions -fkeep-static-functions @gol
--fkeep-static-consts -flive-range-shrinkage @gol
+-fkeep-static-consts -fkeep-gc-roots-live -flive-range-shrinkage @gol
 -floop-block -floop-interchange -floop-strip-mine @gol
 -floop-unroll-and-jam -floop-nest-optimize @gol
 -floop-parallelize-all -flra-remat -flto -flto-compression-level @gol
@@ -6621,6 +6621,17 @@
 If you use @option{-Wunsafe-loop-optimizations}, the compiler warns you
 if it finds this kind of loop.
 
+@item -fkeep-gc-roots-live
+@opindex fkeep-gc-roots-live
+This option tells the compiler that a garbage collector will be used,
+and that therefore the compiled code must retain a live pointer into
+all memory blocks.  The compiler is permitted to construct a pointer
+that is outside the bounds of a memory block, but it must ensure that
+given a pointer into memory, some pointer into that memory remains
+live in the compiled code whenever it is live in the source code.
+This option is disabled by default for most languages, enabled by
+default for languages that use garbage collection.
+
 @item -fcrossjumping
 @opindex fcrossjumping
 Perform cross-jumping transformation.
Index: testsuite/gcc.dg/tree-ssa/ivopt_5.c
===================================================================
--- testsuite/gcc.dg/tree-ssa/ivopt_5.c	(revision 0)
+++ testsuite/gcc.dg/tree-ssa/ivopt_5.c	(working copy)
@@ -0,0 +1,23 @@
+/* { dg-options "-O2 -fdump-tree-ivopts -fkeep-gc-roots-live" } */
+
+/* Only integer ivopts here when using -fkeep-gc-roots-live.   */
+
+void foo (char *pstart, int n)
+{
+  char *p;
+  char *pend = pstart + n;
+
+  for (p = pstart; p < pend; p++)
+    *p = 1;
+}
+
+void foo1 (char *pstart, int n)
+{
+  char *p;
+  char *pend = pstart + n;
+
+  for (p = pstart; p != pend; p++)
+    *p = 1;
+}
+
+/* { dg-final { scan-tree-dump-times "ivtmp.\[0-9_\]* = PHI <\[^0\]" 0 "ivopts"} } */
Index: tree-ssa-loop-ivopts.c
===================================================================
--- tree-ssa-loop-ivopts.c	(revision 232580)
+++ tree-ssa-loop-ivopts.c	(working copy)
@@ -2815,6 +2815,16 @@
   struct iv_cand *cand = NULL;
   tree type, orig_type;
 
+  /* -fkeep-gc-roots-live means that we have to keep a real pointer
+     live, but the ivopts code may replace a real pointer with one
+     pointing before or after the memory block that is then adjusted
+     into the memory block during the loop.  FIXME: It would likely be
+     better to actually force the pointer live and still use ivopts;
+     for example, it would be enough to write the pointer into memory
+     and keep it there until after the loop.  */
+  if (flag_keep_gc_roots_live && POINTER_TYPE_P (TREE_TYPE (base)))
+    return NULL;
+
   /* For non-original variables, make sure their values are computed in a type
      that does not invoke undefined behavior on overflows (since in general,
      we cannot prove that these induction variables are non-wrapping).  */
@@ -3083,8 +3093,11 @@
 	  cand = add_candidate_1 (data,
 				  iv->base, iv->step, true, IP_ORIGINAL, NULL,
 				  SSA_NAME_DEF_STMT (def));
-	  cand->var_before = iv->ssa_name;
-	  cand->var_after = def;
+	  if (cand)
+	    {
+	      cand->var_before = iv->ssa_name;
+	      cand->var_after = def;
+	    }
 	}
       else
 	gcc_assert (gimple_bb (phi) == data->current_loop->header);

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[David Malcolm]

On Tue, 2016-01-26 at 05:35 -0800, Ian Lance Taylor wrote:
[...]

> Index: common.opt
> ===================================================================
> --- common.opt  (revision 232580)
> +++ common.opt  (working copy)
> @@ -1380,6 +1380,10 @@
>  Enable hoisting adjacent loads to encourage generating conditional move
>  instructions.
>  
> +fkeep-gc-roots-live
> +Common Report Var(flag_keep_gc_roots_live) Optimization
> +Always keep a pointer to a live memory block
> +
>  floop-parallelize-all
>  Common Report Var(flag_loop_parallelize_all) Optimization
>  Mark all loops as parallel.
> Index: doc/invoke.texi
> ===================================================================
> --- doc/invoke.texi     (revision 232580)
> +++ doc/invoke.texi     (working copy)
> @@ -359,7 +359,7 @@
>  -fno-ira-share-spill-slots @gol
>  -fisolate-erroneous-paths-dereference -fisolate-erroneous-paths-attribute @gol
>  -fivopts -fkeep-inline-functions -fkeep-static-functions @gol
> --fkeep-static-consts -flive-range-shrinkage @gol
> +-fkeep-static-consts -fkeep-gc-roots-live -flive-range-shrinkage @gol
>  -floop-block -floop-interchange -floop-strip-mine @gol
>  -floop-unroll-and-jam -floop-nest-optimize @gol
>  -floop-parallelize-all -flra-remat -flto -flto-compression-level @gol
> @@ -6621,6 +6621,17 @@
>  If you use @option{-Wunsafe-loop-optimizations}, the compiler warns you
>  if it finds this kind of loop.
>  
> +@item -fkeep-gc-roots-live
> +@opindex fkeep-gc-roots-live
> +This option tells the compiler that a garbage collector will be used,
> +and that therefore the compiled code must retain a live pointer into
> +all memory blocks.  The compiler is permitted to construct a pointer
> +that is outside the bounds of a memory block, but it must ensure that
> +given a pointer into memory, some pointer into that memory remains
> +live in the compiled code whenever it is live in the source code.
> +This option is disabled by default for most languages, enabled by
> +default for languages that use garbage collection.

Is the patch missing some logic to make the option be enabled by default
for gc-using languages?  (presumably go, and maybe java?)

[...snip...]

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Ian Lance Taylor]

On Tue, Jan 26, 2016 at 8:03 AM, David Malcolm <dmalcolm@redhat.com> wrote:
>
> Is the patch missing some logic to make the option be enabled by default
> for gc-using languages?  (presumably go, and maybe java?)

I am intentionally leaving that to a separate patch, yes.  I think
this option is useful by itself for C/C++ programs that intend to use
something like the Boehm collector.  If the option is not useful by
itself, then a different approach may be appropriate.

Ian

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Bernd Schmidt]

> +This option is disabled by default for most languages, enabled by
> +default for languages that use garbage collection.

This is not true as of this patch.

> Index: tree-ssa-loop-ivopts.c
> ===================================================================
> --- tree-ssa-loop-ivopts.c	(revision 232580)
> +++ tree-ssa-loop-ivopts.c	(working copy)
> @@ -2815,6 +2815,16 @@
>     struct iv_cand *cand = NULL;
>     tree type, orig_type;
>
> +  /* -fkeep-gc-roots-live means that we have to keep a real pointer
> +     live, but the ivopts code may replace a real pointer with one
> +     pointing before or after the memory block that is then adjusted
> +     into the memory block during the loop.  FIXME: It would likely be
> +     better to actually force the pointer live and still use ivopts;
> +     for example, it would be enough to write the pointer into memory
> +     and keep it there until after the loop.  */
> +  if (flag_keep_gc_roots_live && POINTER_TYPE_P (TREE_TYPE (base)))
> +    return NULL;

Is there a reason to exclude a candidate if the base is just a pointer 
to an object? Or does it just not make a difference?

I feel somewhat uneasy that this probably isn't a full solution either. 
But it seems like a reasonable starting point. I'm still not sure we 
want to advertise the option to users, as it might overpromise at the 
moment. Do we have precedent for internal-only options?

Or maybe we do need some other form to represent the need to keep a 
pointer alive in the IL. I suspect that for gcc-6 your patch is probably 
reasonable, but we should mark the option as internal and likely to go 
away in the future.


Bernd

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Ian Lance Taylor]

On Wed, Jan 27, 2016 at 3:18 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>> +This option is disabled by default for most languages, enabled by
>> +default for languages that use garbage collection.
>
>
> This is not true as of this patch.

Yes.  As I said elsewhere, my intent is to do that as a separate patch.


>> Index: tree-ssa-loop-ivopts.c
>> ===================================================================
>> --- tree-ssa-loop-ivopts.c      (revision 232580)
>> +++ tree-ssa-loop-ivopts.c      (working copy)
>> @@ -2815,6 +2815,16 @@
>>     struct iv_cand *cand = NULL;
>>     tree type, orig_type;
>>
>> +  /* -fkeep-gc-roots-live means that we have to keep a real pointer
>> +     live, but the ivopts code may replace a real pointer with one
>> +     pointing before or after the memory block that is then adjusted
>> +     into the memory block during the loop.  FIXME: It would likely be
>> +     better to actually force the pointer live and still use ivopts;
>> +     for example, it would be enough to write the pointer into memory
>> +     and keep it there until after the loop.  */
>> +  if (flag_keep_gc_roots_live && POINTER_TYPE_P (TREE_TYPE (base)))
>> +    return NULL;
>
>
> Is there a reason to exclude a candidate if the base is just a pointer to an
> object? Or does it just not make a difference?

Off hand I don't see how it would make a difference.  If the pointer
is not incremented or indexed, ivopts isn't going to be able to do
anything with it anyhow.


> I feel somewhat uneasy that this probably isn't a full solution either. But
> it seems like a reasonable starting point. I'm still not sure we want to
> advertise the option to users, as it might overpromise at the moment. Do we
> have precedent for internal-only options?

I suppose an option marked as "Undocumented" could be considered to be
internal-only.

The option does in principle over-promise.  On the other hand, it's
probably pretty good in practice; Java and Go have gotten by without
this option for many years.  I only have a single test case that shows
a real problem, and it only shows up on PPC64.  That said, I'm fine
with making the option undocumented if you prefer.

> Or maybe we do need some other form to represent the need to keep a pointer
> alive in the IL. I suspect that for gcc-6 your patch is probably reasonable,
> but we should mark the option as internal and likely to go away in the
> future.

Why would the option  be likely to go away?  The option name is chosen
(based on Richi's suggestion) so that it can be applied to other
passes besides ivopts, and it's useful for languages that do not
normally do garbage collection.  If we improve the implementation as
you suggest, it would still be reasonable to keep the option.

Ian

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Bernd Schmidt]

On 01/27/2016 02:59 PM, Ian Lance Taylor wrote:
>>> +This option is disabled by default for most languages, enabled by
>>> +default for languages that use garbage collection.
>>
>>
>> This is not true as of this patch.
>
> Yes.  As I said elsewhere, my intent is to do that as a separate patch.

Then the followup patch should adjust the documentation.

> The option does in principle over-promise.  On the other hand, it's
> probably pretty good in practice; Java and Go have gotten by without
> this option for many years.  I only have a single test case that shows
> a real problem, and it only shows up on PPC64.  That said, I'm fine
> with making the option undocumented if you prefer.
>
>> Or maybe we do need some other form to represent the need to keep a pointer
>> alive in the IL. I suspect that for gcc-6 your patch is probably reasonable,
>> but we should mark the option as internal and likely to go away in the
>> future.
>
> Why would the option  be likely to go away?  The option name is chosen
> (based on Richi's suggestion) so that it can be applied to other
> passes besides ivopts, and it's useful for languages that do not
> normally do garbage collection.  If we improve the implementation as
> you suggest, it would still be reasonable to keep the option.

I think it would be better described in the IL, so that in an LTO 
situation the restrictions would apply only to code which needs it. 
Although I suppose you could then enable it for C and make that generate 
the necessary representation.

Still, I feel uncomfortable about making a promise we don't really 
expect to fully keep yet, so I would prefer this option to be 
undocumented for now. I won't object if someone else wants to approve it 
as a normal option however.


Bernd

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Ian Lance Taylor]

On Wed, Jan 27, 2016 at 7:16 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>
> Still, I feel uncomfortable about making a promise we don't really expect to
> fully keep yet, so I would prefer this option to be undocumented for now. I
> won't object if someone else wants to approve it as a normal option however.

Are you approving the patch if I change the option to be undocumented?

Ian

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Bernd Schmidt]

On 01/27/2016 04:18 PM, Ian Lance Taylor wrote:
> On Wed, Jan 27, 2016 at 7:16 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>>
>> Still, I feel uncomfortable about making a promise we don't really expect to
>> fully keep yet, so I would prefer this option to be undocumented for now. I
>> won't object if someone else wants to approve it as a normal option however.
>
> Are you approving the patch if I change the option to be undocumented?

Yes.


Bernd

Re: Patch RFA: Add option -fcollectible-pointers, use it in ivopts

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]

On Wed, Jan 27, 2016 at 7:24 AM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 01/27/2016 04:18 PM, Ian Lance Taylor wrote:
>>
>> On Wed, Jan 27, 2016 at 7:16 AM, Bernd Schmidt <bschmidt@redhat.com>
>> wrote:
>>>
>>>
>>> Still, I feel uncomfortable about making a promise we don't really expect
>>> to
>>> fully keep yet, so I would prefer this option to be undocumented for now.
>>> I
>>> won't object if someone else wants to approve it as a normal option
>>> however.
>>
>>
>> Are you approving the patch if I change the option to be undocumented?
>
>
> Yes.

Committed as follows.

gcc/ChangeLog:

2016-01-27  Ian Lance Taylor  <iant@google.com>

* common.opt (fkeep-gc-roots-live): New undocumented option.
* tree-ssa-loop-ivopts.c (add_candidate_1): If
-fkeep-gc-roots-live, skip pointers.
(add_iv_candidate_for_biv): Handle add_candidate_1 returning
NULL.

gcc/testsuite/ChangeLog:

2016-01-27  Ian Lance Taylor  <iant@google.com>

* gcc.dg/tree-ssa/ivopt_5.c: New test.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2720 bytes --]

Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 232580)
+++ gcc/common.opt	(working copy)
@@ -1380,6 +1380,10 @@
 Enable hoisting adjacent loads to encourage generating conditional move
 instructions.
 
+fkeep-gc-roots-live
+Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization
+; Always keep a pointer to a live memory block
+
 floop-parallelize-all
 Common Report Var(flag_loop_parallelize_all) Optimization
 Mark all loops as parallel.
Index: gcc/tree-ssa-loop-ivopts.c
===================================================================
--- gcc/tree-ssa-loop-ivopts.c	(revision 232580)
+++ gcc/tree-ssa-loop-ivopts.c	(working copy)
@@ -2815,6 +2815,16 @@
   struct iv_cand *cand = NULL;
   tree type, orig_type;
 
+  /* -fkeep-gc-roots-live means that we have to keep a real pointer
+     live, but the ivopts code may replace a real pointer with one
+     pointing before or after the memory block that is then adjusted
+     into the memory block during the loop.  FIXME: It would likely be
+     better to actually force the pointer live and still use ivopts;
+     for example, it would be enough to write the pointer into memory
+     and keep it there until after the loop.  */
+  if (flag_keep_gc_roots_live && POINTER_TYPE_P (TREE_TYPE (base)))
+    return NULL;
+
   /* For non-original variables, make sure their values are computed in a type
      that does not invoke undefined behavior on overflows (since in general,
      we cannot prove that these induction variables are non-wrapping).  */
@@ -3083,8 +3093,11 @@
 	  cand = add_candidate_1 (data,
 				  iv->base, iv->step, true, IP_ORIGINAL, NULL,
 				  SSA_NAME_DEF_STMT (def));
-	  cand->var_before = iv->ssa_name;
-	  cand->var_after = def;
+	  if (cand)
+	    {
+	      cand->var_before = iv->ssa_name;
+	      cand->var_after = def;
+	    }
 	}
       else
 	gcc_assert (gimple_bb (phi) == data->current_loop->header);
Index: gcc/testsuite/gcc.dg/tree-ssa/ivopt_5.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/ivopt_5.c	(revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/ivopt_5.c	(working copy)
@@ -0,0 +1,23 @@
+/* { dg-options "-O2 -fdump-tree-ivopts -fkeep-gc-roots-live" } */
+
+/* Only integer ivopts here when using -fkeep-gc-roots-live.   */
+
+void foo (char *pstart, int n)
+{
+  char *p;
+  char *pend = pstart + n;
+
+  for (p = pstart; p < pend; p++)
+    *p = 1;
+}
+
+void foo1 (char *pstart, int n)
+{
+  char *p;
+  char *pend = pstart + n;
+
+  for (p = pstart; p != pend; p++)
+    *p = 1;
+}
+
+/* { dg-final { scan-tree-dump-times "ivtmp.\[0-9_\]* = PHI <\[^0\]" 0 "ivopts"} } */