[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
>