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_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 (&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