On Thu, Jan 28, 2016 at 6:49 PM, Tom de Vries <tom_devr...@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_devr...@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_devr...@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 (®_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 >