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

Message ID CAFiYyc19ADh6smW-T4t+QF2_n-g8mPMMzMK2RbaPZO8gz754Ow@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener Jan. 28, 2016, 1:32 p.m. UTC
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:

Richard.

> Thanks,
> - Tom

Comments

Tom de Vries Jan. 28, 2016, 5:49 p.m. UTC | #1
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
Richard Biener Jan. 29, 2016, 7:48 a.m. UTC | #2
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
>
Nathan Sidwell Jan. 29, 2016, 2:16 p.m. UTC | #3
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

Patch
diff mbox

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);