Re: [gomp4] acc routines bugfix

[Cesar Philippidis <cesar at codesourcery dot com>]

On 07/24/2015 08:21 AM, Ilya Verbin wrote:
> On Fri, Jul 24, 2015 at 08:05:00 -0700, Cesar Philippidis wrote:
>> The second point is interesting. Offloaded functions require the "omp
>> target" attribute or that function won't reach the lto compiler. That's
>> fine because not all targets can handle general code. The problem occurs
>> when a user forgets to bless a function as offloaded, which OpenACC
>> allows. This patch teaches the lto-wrapper to error on unrecognized
>> functions with flag_openacc or hit gcc_unreachable otherwise. I couldn't
>> think of a way to test the lto error message because that involves
>> having two compilers present. I wonder if it's ok to have libgomp check
>> for compiler expected compiler errors? However, that's more of a
>> gcc/testsuite type of check.
>>
>> I don't think trunk has much support for acc routines just yet, so I
>> applied this patch to gomp-4_0-branch for now.
> 
> OpenMP has similar issue.
> 
>> diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
>> index 97585c9..bc589bd 100644
>> --- a/gcc/lto-cgraph.c
>> +++ b/gcc/lto-cgraph.c
>> @@ -1219,9 +1219,23 @@ input_overwrite_node (struct lto_file_decl_data *file_data,
>>  				     LDPR_NUM_KNOWN);
>>    node->instrumentation_clone = bp_unpack_value (bp, 1);
>>    node->split_part = bp_unpack_value (bp, 1);
>> -  gcc_assert (flag_ltrans
>> -	      || (!node->in_other_partition
>> -		  && !node->used_from_other_partition));
>> +
>> +  int success = flag_ltrans || (!node->in_other_partition
>> +				&& !node->used_from_other_partition);
>> +
>> +  if (!success)
>> +    {
>> +      if (flag_openacc)
>> +	{
>> +	  if (TREE_CODE (node->decl) == FUNCTION_DECL)
>> +	    error ("Missing routine function %<%s%>", node->name ());
>> +	  else
>> +	    error ("Missing declared variable %<%s%>", node->name ());
>> +	}
>> +
>> +      else
>> +	gcc_unreachable ();
>> +    }
>>  }
> 
> This will print an error not only when a fn/var, referenced from offload region,
> missed its attribute, but also when something goes wrong in general LTO
> partitioning (if flag_openacc is set).  So, maybe just replace gcc_assert ()
> with error () without checking for flag_openacc?
> 
> And how about similar assert in input_varpool_node?

Good catch. I've been too focused on OpenACC lately. Would this patch be
OK for trunk if it passes testing?

Cesar

2015-07-24  Cesar Philippidis  <cesar@codesourcery.com>

	gcc/
	* lto-cgraph.c (input_overwrite_node): Error instead of assert
	on missing cgraph partitions.
	(input_varpool_node): Likewise.


diff --git a/gcc/lto-cgraph.c b/gcc/lto-cgraph.c
index d70537d..7e2fc80 100644
--- a/gcc/lto-cgraph.c
+++ b/gcc/lto-cgraph.c
@@ -1218,9 +1218,11 @@ input_overwrite_node (struct lto_file_decl_data *file_data,
 				     LDPR_NUM_KNOWN);
   node->instrumentation_clone = bp_unpack_value (bp, 1);
   node->split_part = bp_unpack_value (bp, 1);
-  gcc_assert (flag_ltrans
-	      || (!node->in_other_partition
-		  && !node->used_from_other_partition));
+
+  int success = flag_ltrans || (!node->in_other_partition
+				&& !node->used_from_other_partition);
+  if (!success)
+    error ("Missing %<%s%>", node->name ());
 }
 
 /* Return string alias is alias of.  */
@@ -1432,9 +1434,11 @@ input_varpool_node (struct lto_file_decl_data *file_data,
     node->set_section_for_node (section);
   node->resolution = streamer_read_enum (ib, ld_plugin_symbol_resolution,
 					        LDPR_NUM_KNOWN);
-  gcc_assert (flag_ltrans
-	      || (!node->in_other_partition
-		  && !node->used_from_other_partition));
+
+  int success = flag_ltrans || (!node->in_other_partition
+				&& !node->used_from_other_partition);
+  if (!success)
+    error ("Missing %<%s%>", node->name ());
 
   return node;
 }