[gomp4, PR68977, Committed] Don't gimplify in ssa mode if seen_error in oacc_xform_loop

[gomp4, PR68977, Committed] Don't gimplify in ssa mode if seen_error in oacc_xform_loop

[Tom de Vries]

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

Hi,

At r231739, there was an ICE when checking code generated by 
oacc_xform_loop, in case the source contained an error.

Due to seen_error (), gimplification during oacc_xform_loop bailed out, 
and an uninitialized var was introduced.  Because of gimplifying in ssa 
mode, that caused an ICE.

I can't reproduce this any longer, but I think the fix still makes 
sense. The patch makes sure oacc_xform_loop gimplifies in non-ssa mode 
if seen_error ().

Build on x86_64 with nvidia accelerator setup, tested libgomp and goacc.exp.

Committed to gomp-4_0-branch.

Thanks,
- Tom

[-- Attachment #2: 0001-Don-t-gimplify-in-ssa-mode-if-seen_error-in-oacc_xform_loop.patch --]
[-- Type: text/x-patch, Size: 1029 bytes --]

Don't gimplify in ssa mode if seen_error in oacc_xform_loop

2016-01-13  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/68977
	* omp-low.c (oacc_xform_loop): Handle seen_error () == true.
---
 gcc/ChangeLog.gomp | 5 +++++
 gcc/omp-low.c      | 7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index c99f0b8..a6e3fe3 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -19105,7 +19105,12 @@ oacc_xform_loop (gcall *call)
        -> chunks=ceil (range/(chunksize*threads*step))
      striding=false,chunking=false
        -> chunk_size=ceil(range/(threads*step)),chunks=1  */
-  push_gimplify_context (true);
+
+  /* If seen_error (), we may introduce an uninitialized var due to
+     gimplification bailing out.  If we gimplify in ssa mode, that will cause an
+     ICE.  If we gimplify in non-ssa mode, then ssa updating will turn it into a
+     default definition, and we avoid the ICE.  */
+  push_gimplify_context (!seen_error ());
 
   switch (code)
     {

Re: [gomp4, PR68977, Committed] Don't gimplify in ssa mode if seen_error in oacc_xform_loop

[Richard Biener]

On Wed, Jan 13, 2016 at 9:04 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> Hi,
>
> At r231739, there was an ICE when checking code generated by
> oacc_xform_loop, in case the source contained an error.
>
> Due to seen_error (), gimplification during oacc_xform_loop bailed out, and
> an uninitialized var was introduced.  Because of gimplifying in ssa mode,
> that caused an ICE.
>
> I can't reproduce this any longer, but I think the fix still makes sense.
> The patch makes sure oacc_xform_loop gimplifies in non-ssa mode if
> seen_error ().

I don't think it makes "sense" in any way.  After seen_error () a following ICE
will be "confused after earlier errors" in release mode and thus I think that's
not an important problem to paper over with this kind of "hack".

I'd rather avoid doing any of omp-low if seen_error ()?

Richard.

> Build on x86_64 with nvidia accelerator setup, tested libgomp and goacc.exp.
>
> Committed to gomp-4_0-branch.
>
> Thanks,
> - Tom

Re: [gomp4, PR68977, Committed] Don't gimplify in ssa mode if seen_error in oacc_xform_loop

[Tom de Vries]

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

On 14/01/16 10:43, Richard Biener wrote:
> On Wed, Jan 13, 2016 at 9:04 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> Hi,
>>
>> At r231739, there was an ICE when checking code generated by
>> oacc_xform_loop, in case the source contained an error.
>>
>> Due to seen_error (), gimplification during oacc_xform_loop bailed out, and
>> an uninitialized var was introduced.  Because of gimplifying in ssa mode,
>> that caused an ICE.
>>
>> I can't reproduce this any longer, but I think the fix still makes sense.
>> The patch makes sure oacc_xform_loop gimplifies in non-ssa mode if
>> seen_error ().
>
> I don't think it makes "sense" in any way.  After seen_error () a following ICE
> will be "confused after earlier errors" in release mode and thus I think that's
> not an important problem to paper over with this kind of "hack".
>
> I'd rather avoid doing any of omp-low if seen_error ()?
>

The error triggered in oacc_device_lower, so there's nothing we can do 
before (in omp-low).

How about this fix, which replaces the oacc ifn calls with 
zero-assignments if seen_error ()?

Thanks,
- Tom

[-- Attachment #2: 0001-Ignore-oacc-ifn-if-seen_error-in-execute_oacc_device_lower.patch --]
[-- Type: text/x-patch, Size: 2113 bytes --]

Ignore oacc ifn if seen_error in execute_oacc_device_lower

---
 gcc/omp-low.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index 2de3aeb..f678f05 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -20201,7 +20201,7 @@ execute_oacc_device_lower ()
 
 	/* Rewind to allow rescan.  */
 	gsi_prev (&gsi);
-	bool rescan = false, remove = false;
+	bool rescan = false, remove = false, assign_zero = false;
 	enum  internal_fn ifn_code = gimple_call_internal_fn (call);
 
 	switch (ifn_code)
@@ -20209,11 +20209,25 @@ execute_oacc_device_lower ()
 	  default: break;
 
 	  case IFN_GOACC_LOOP:
+	    if (seen_error ())
+	      {
+		remove = true;
+		assign_zero = true;
+		break;
+	      }
+
 	    oacc_xform_loop (call);
 	    rescan = true;
 	    break;
 
 	  case IFN_GOACC_REDUCTION:
+	    if (seen_error ())
+	      {
+		remove = true;
+		assign_zero = true;
+		break;
+	      }
+
 	    /* Mark the function for SSA renaming.  */
 	    mark_virtual_operands_for_renaming (cfun);
 
@@ -20228,6 +20242,13 @@ execute_oacc_device_lower ()
 
 	  case IFN_UNIQUE:
 	    {
+	      if (seen_error ())
+		{
+		  remove = true;
+		  assign_zero = true;
+		  break;
+		}
+
 	      enum ifn_unique_kind kind
 		= ((enum ifn_unique_kind)
 		   TREE_INT_CST_LOW (gimple_call_arg (call, 0)));
@@ -20266,11 +20287,19 @@ execute_oacc_device_lower ()
 	  {
 	    if (gimple_vdef (call))
 	      replace_uses_by (gimple_vdef (call), gimple_vuse (call));
-	    if (gimple_call_lhs (call))
+	    tree lhs = gimple_call_lhs (call);
+	    if (lhs != NULL_TREE)
 	      {
-		/* Propagate the data dependency var.  */
-		gimple *ass = gimple_build_assign (gimple_call_lhs (call),
-						   gimple_call_arg (call, 1));
+		gimple *ass;
+		if (assign_zero)
+		  {
+		    tree zero = build_zero_cst (TREE_TYPE (lhs));
+		    ass = gimple_build_assign (lhs, zero);
+		  }
+		else
+		  /* Propagate the data dependency var.  */
+		  ass = gimple_build_assign (lhs, gimple_call_arg (call, 1));
+
 		gsi_replace (&gsi, ass,  false);
 	      }
 	    else

Re: [gomp4, PR68977, Committed] Don't gimplify in ssa mode if seen_error in oacc_xform_loop

[Richard Biener]

On Mon, Jan 25, 2016 at 12:00 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 14/01/16 10:43, Richard Biener wrote:
>>
>> On Wed, Jan 13, 2016 at 9:04 PM, Tom de Vries <Tom_deVries@mentor.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> At r231739, there was an ICE when checking code generated by
>>> oacc_xform_loop, in case the source contained an error.
>>>
>>> Due to seen_error (), gimplification during oacc_xform_loop bailed out,
>>> and
>>> an uninitialized var was introduced.  Because of gimplifying in ssa mode,
>>> that caused an ICE.
>>>
>>> I can't reproduce this any longer, but I think the fix still makes sense.
>>> The patch makes sure oacc_xform_loop gimplifies in non-ssa mode if
>>> seen_error ().
>>
>>
>> I don't think it makes "sense" in any way.  After seen_error () a
>> following ICE
>> will be "confused after earlier errors" in release mode and thus I think
>> that's
>> not an important problem to paper over with this kind of "hack".
>>
>> I'd rather avoid doing any of omp-low if seen_error ()?
>>
>
> The error triggered in oacc_device_lower, so there's nothing we can do
> before (in omp-low).
>
> How about this fix, which replaces the oacc ifn calls with zero-assignments
> if seen_error ()?

Again it looks like too much complexity for an ICE-after-error which will
be reported as "confused after errors" only.  I'd accept a patch that simply
stops processing the function before lowering which is what we usually
do with this kind of cases [yes, it might make sense to start tracking
seen-error per function].

So in this case add a seen_errors () guard to cgraph_node::expand ()
like the following:

Index: cgraphunit.c
===================================================================
--- cgraphunit.c        (revision 232666)
+++ cgraphunit.c        (working copy)
@@ -1967,15 +1967,18 @@ cgraph_node::expand (void)

   execute_all_ipa_transforms ();

-  /* Perform all tree transforms and optimizations.  */
-
-  /* Signal the start of passes.  */
-  invoke_plugin_callbacks (PLUGIN_ALL_PASSES_START, NULL);
-
-  execute_pass_list (cfun, g->get_passes ()->all_passes);
-
-  /* Signal the end of passes.  */
-  invoke_plugin_callbacks (PLUGIN_ALL_PASSES_END, NULL);
+  if (! seen_error ())
+    {
+      /* Perform all tree transforms and optimizations.  */
+
+      /* Signal the start of passes.  */
+      invoke_plugin_callbacks (PLUGIN_ALL_PASSES_START, NULL);
+
+      execute_pass_list (cfun, g->get_passes ()->all_passes);
+
+      /* Signal the end of passes.  */
+      invoke_plugin_callbacks (PLUGIN_ALL_PASSES_END, NULL);
+    }

   bitmap_obstack_release (&reg_obstack);

Richard.

> Thanks,
> - Tom

Re: [gomp4, PR68977, Committed] Don't gimplify in ssa mode if seen_error in oacc_xform_loop

[Tom de Vries]

On 28/01/16 14:32, Richard Biener wrote:
> On Mon, Jan 25, 2016 at 12:00 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> On 14/01/16 10:43, Richard Biener wrote:
>>>
>>> On Wed, Jan 13, 2016 at 9:04 PM, Tom de Vries <Tom_deVries@mentor.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> At r231739, there was an ICE when checking code generated by
>>>> oacc_xform_loop, in case the source contained an error.
>>>>
>>>> Due to seen_error (), gimplification during oacc_xform_loop bailed out,
>>>> and
>>>> an uninitialized var was introduced.  Because of gimplifying in ssa mode,
>>>> that caused an ICE.
>>>>
>>>> I can't reproduce this any longer, but I think the fix still makes sense.
>>>> The patch makes sure oacc_xform_loop gimplifies in non-ssa mode if
>>>> seen_error ().
>>>
>>>
>>> I don't think it makes "sense" in any way.  After seen_error () a
>>> following ICE
>>> will be "confused after earlier errors" in release mode and thus I think
>>> that's
>>> not an important problem to paper over with this kind of "hack".
>>>
>>> I'd rather avoid doing any of omp-low if seen_error ()?
>>>
>>
>> The error triggered in oacc_device_lower, so there's nothing we can do
>> before (in omp-low).
>>
>> How about this fix, which replaces the oacc ifn calls with zero-assignments
>> if seen_error ()?
>
> Again it looks like too much complexity for an ICE-after-error which will
> be reported as "confused after errors" only.

IMO it's not much different from what is done in lower_omp_1:
...
   /* If we have issued syntax errors, avoid doing any heavy lifting.
      Just replace the OMP directives with a NOP to avoid
      confusing RTL expansion.  */
   if (seen_error () && is_gimple_omp (stmt))
     {
       gsi_replace (gsi_p, gimple_build_nop (), true);
       return;
     }
...

> I'd accept a patch that simply
> stops processing the function before lowering which is what we usually
> do with this kind of cases [yes, it might make sense to start tracking
> seen-error per function].
>

[ AFAIU, the patch below stops after lowering. ]

> So in this case add a seen_errors () guard to cgraph_node::expand ()
> like the following:
>
> Index: cgraphunit.c
> ===================================================================
> --- cgraphunit.c        (revision 232666)
> +++ cgraphunit.c        (working copy)
> @@ -1967,15 +1967,18 @@ cgraph_node::expand (void)
>
>     execute_all_ipa_transforms ();
>
> -  /* Perform all tree transforms and optimizations.  */
> -
> -  /* Signal the start of passes.  */
> -  invoke_plugin_callbacks (PLUGIN_ALL_PASSES_START, NULL);
> -
> -  execute_pass_list (cfun, g->get_passes ()->all_passes);
> -
> -  /* Signal the end of passes.  */
> -  invoke_plugin_callbacks (PLUGIN_ALL_PASSES_END, NULL);
> +  if (! seen_error ())
> +    {
> +      /* Perform all tree transforms and optimizations.  */
> +
> +      /* Signal the start of passes.  */
> +      invoke_plugin_callbacks (PLUGIN_ALL_PASSES_START, NULL);
> +
> +      execute_pass_list (cfun, g->get_passes ()->all_passes);
> +
> +      /* Signal the end of passes.  */
> +      invoke_plugin_callbacks (PLUGIN_ALL_PASSES_END, NULL);
> +    }
>
>     bitmap_obstack_release (&reg_obstack);

I suppose the patch makes sense in general.

But it doesn't address the scenario I'm trying to fix: 
pass_oacc_device_lower signals an error, and then may run into an ICE 
during gimplification in that same pass.

What would work (and is less fine-grained than the solutions I've 
proposed until now) is bailing out of pass_oacc_device_lower once 
seen_error, before doing any gimplification, and then not running any 
further passes, to prevent running into further ICEs.

Thanks,
- Tom

Re: [gomp4, PR68977, Committed] Don't gimplify in ssa mode if seen_error in oacc_xform_loop

[Richard Biener]

On Thu, Jan 28, 2016 at 6:49 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 28/01/16 14:32, Richard Biener wrote:
>>
>> On Mon, Jan 25, 2016 at 12:00 PM, Tom de Vries <Tom_deVries@mentor.com>
>> wrote:
>>>
>>> On 14/01/16 10:43, Richard Biener wrote:
>>>>
>>>>
>>>> On Wed, Jan 13, 2016 at 9:04 PM, Tom de Vries <Tom_deVries@mentor.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> At r231739, there was an ICE when checking code generated by
>>>>> oacc_xform_loop, in case the source contained an error.
>>>>>
>>>>> Due to seen_error (), gimplification during oacc_xform_loop bailed out,
>>>>> and
>>>>> an uninitialized var was introduced.  Because of gimplifying in ssa
>>>>> mode,
>>>>> that caused an ICE.
>>>>>
>>>>> I can't reproduce this any longer, but I think the fix still makes
>>>>> sense.
>>>>> The patch makes sure oacc_xform_loop gimplifies in non-ssa mode if
>>>>> seen_error ().
>>>>
>>>>
>>>>
>>>> I don't think it makes "sense" in any way.  After seen_error () a
>>>> following ICE
>>>> will be "confused after earlier errors" in release mode and thus I think
>>>> that's
>>>> not an important problem to paper over with this kind of "hack".
>>>>
>>>> I'd rather avoid doing any of omp-low if seen_error ()?
>>>>
>>>
>>> The error triggered in oacc_device_lower, so there's nothing we can do
>>> before (in omp-low).
>>>
>>> How about this fix, which replaces the oacc ifn calls with
>>> zero-assignments
>>> if seen_error ()?
>>
>>
>> Again it looks like too much complexity for an ICE-after-error which will
>> be reported as "confused after errors" only.
>
>
> IMO it's not much different from what is done in lower_omp_1:
> ...
>   /* If we have issued syntax errors, avoid doing any heavy lifting.
>      Just replace the OMP directives with a NOP to avoid
>      confusing RTL expansion.  */
>   if (seen_error () && is_gimple_omp (stmt))
>     {
>       gsi_replace (gsi_p, gimple_build_nop (), true);
>       return;
>     }
> ...
>
>> I'd accept a patch that simply
>> stops processing the function before lowering which is what we usually
>> do with this kind of cases [yes, it might make sense to start tracking
>> seen-error per function].
>>
>
> [ AFAIU, the patch below stops after lowering. ]
>
>
>> So in this case add a seen_errors () guard to cgraph_node::expand ()
>> like the following:
>>
>> Index: cgraphunit.c
>> ===================================================================
>> --- cgraphunit.c        (revision 232666)
>> +++ cgraphunit.c        (working copy)
>> @@ -1967,15 +1967,18 @@ cgraph_node::expand (void)
>>
>>     execute_all_ipa_transforms ();
>>
>> -  /* Perform all tree transforms and optimizations.  */
>> -
>> -  /* Signal the start of passes.  */
>> -  invoke_plugin_callbacks (PLUGIN_ALL_PASSES_START, NULL);
>> -
>> -  execute_pass_list (cfun, g->get_passes ()->all_passes);
>> -
>> -  /* Signal the end of passes.  */
>> -  invoke_plugin_callbacks (PLUGIN_ALL_PASSES_END, NULL);
>> +  if (! seen_error ())
>> +    {
>> +      /* Perform all tree transforms and optimizations.  */
>> +
>> +      /* Signal the start of passes.  */
>> +      invoke_plugin_callbacks (PLUGIN_ALL_PASSES_START, NULL);
>> +
>> +      execute_pass_list (cfun, g->get_passes ()->all_passes);
>> +
>> +      /* Signal the end of passes.  */
>> +      invoke_plugin_callbacks (PLUGIN_ALL_PASSES_END, NULL);
>> +    }
>>
>>     bitmap_obstack_release (&reg_obstack);
>
>
> I suppose the patch makes sense in general.
>
> But it doesn't address the scenario I'm trying to fix:
> pass_oacc_device_lower signals an error, and then may run into an ICE during
> gimplification in that same pass.
>
> What would work (and is less fine-grained than the solutions I've proposed
> until now) is bailing out of pass_oacc_device_lower once seen_error, before
> doing any gimplification, and then not running any further passes, to
> prevent running into further ICEs.

I see.  Is it possible to simply scrub the whole OACC region in this
case instead?
Or even better, report those errors earlier?

Richard.

> Thanks,
> - Tom
>

Re: [gomp4, PR68977, Committed] Don't gimplify in ssa mode if seen_error in oacc_xform_loop

[Nathan Sidwell]

On 01/29/16 02:48, Richard Biener wrote:

> I see.  Is it possible to simply scrub the whole OACC region in this
> case instead?

Do you mean jettison the body of the offloaded fn, or something else?  I guess 
the former's doable.  (Throwing away the fn entirely could result in unresolved 
symbol errors, which might confuse?)

> Or even better, report those errors earlier?

Well, one could split the pass into two passes (I think) and move the first 
half.  But in general, these errors are only discoverable in the device compiler.

nathan
-- 
Nathan Sidwell - Director, Sourcery Services - Mentor Embedded