[PING] options: Clarify 'Init' option property usage for streaming optimization (was: [PATCH] options, lto: Optimize streaming of optimization nodes)

Message ID 87eduud7eg.fsf@dem-tschwing-1.ger.mentorg.com
State Accepted
Headers
Series [PING] options: Clarify 'Init' option property usage for streaming optimization (was: [PATCH] options, lto: Optimize streaming of optimization nodes) |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Thomas Schwinge Oct. 26, 2022, 1:46 p.m. UTC
  Hi!

Ping.  For convenience, I've re-attached
"options: Clarify 'Init' option property usage for streaming optimization".
(I've re-verified: "No functional change; no change in generated files.")


Grüße
 Thomas


On 2022-03-31T15:22:59+0200, I wrote:
> Hi!
>
> On 2020-11-18T10:36:35+0100, Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>> Honza mentioned that especially for the new param machinery, most of
>> streamed values are probably going to be the default values.  Perhaps
>> somehow we could stream them more effectively.
>>
>> This patch implements it and brings further savings, the size
>> goes down from 574 bytes to 273 bytes, i.e. less than half.
>
> Neat idea about the XOR-encoding!
>
>> Not trying to handle enums because the code doesn't know if (enum ...) 10
>> is even valid, similarly non-parameters because those really generally
>> don't have large initializers, and params without Init (those are 0
>> initialized and thus don't need to be handled).
>
> Given this only looks at 'Init', I understand this may actually
> mis-optimize in the case that there is an 'Init' present, but that value
> is then "by default" overridden via 'LANG_HOOKS_INIT_OPTIONS_STRUCT', or
> 'TARGET_OPTION_OVERRIDE', for example.  (I'm however not claiming that's
> an actual problem to be worried about -- just for my understanding.)
>
> Either way:
>
>>      * optc-save-gen.awk: Initialize var_opt_init.  In
>>      cl_optimization_stream_out for params with default values larger than
>>      10, xor the default value with the actual parameter value.  In
>>      cl_optimization_stream_in repeat the above xor.
>
>> --- gcc/optc-save-gen.awk.jj 2020-09-14 10:51:54.493740942 +0200
>> +++ gcc/optc-save-gen.awk    2020-09-14 11:39:39.441602594 +0200
>> @@ -1186,6 +1186,7 @@ for (i = 0; i < n_opts; i++) {
>>              var_opt_val_type[n_opt_val] = otype;
>>              var_opt_val[n_opt_val] = "x_" name;
>>              var_opt_hash[n_opt_val] = flag_set_p("Optimization", flags[i]);
>> +            var_opt_init[n_opt_val] = opt_args("Init", flags[i]);
>>              n_opt_val++;
>>      }
>>  }
>
> Reading through the options handling, I was confused why 'Init' needs to
> be handled in 'gcc/optc-save-gen.awk'.  To help the next person -- like,
> me in a few weeks ;-) -- wondering about this, OK to push the attached
> "options: Clarify 'Init' option property usage for streaming
> optimization"?
>
>
> Grüße
>  Thomas
>
>
>> @@ -1257,10 +1258,21 @@ for (i = 0; i < n_opt_val; i++) {
>>      otype = var_opt_val_type[i];
>>      if (otype ~ "^const char \\**$")
>>              print "  bp_pack_string (ob, bp, ptr->" name", true);";
>> -    else if (otype ~ "^unsigned")
>> -            print "  bp_pack_var_len_unsigned (bp, ptr->" name");";
>> -    else
>> -            print "  bp_pack_var_len_int (bp, ptr->" name");";
>> +    else {
>> +            if (otype ~ "^unsigned") {
>> +                    sgn = "unsigned";
>> +            } else {
>> +                    sgn = "int";
>> +            }
>> +            if (name ~ "^x_param" && !(otype ~ "^enum ") && var_opt_init[i]) {
>> +                    print "  if (" var_opt_init[i] " > (" var_opt_val_type[i] ") 10)";
>> +                    print "    bp_pack_var_len_" sgn " (bp, ptr->" name" ^ " var_opt_init[i] ");";
>> +                    print "  else";
>> +                    print "    bp_pack_var_len_" sgn " (bp, ptr->" name");";
>> +            } else {
>> +                    print "  bp_pack_var_len_" sgn " (bp, ptr->" name");";
>> +            }
>> +    }
>>  }
>>  print "  for (size_t i = 0; i < sizeof (ptr->explicit_mask) / sizeof (ptr->explicit_mask[0]); i++)";
>>  print "    bp_pack_value (bp, ptr->explicit_mask[i], 64);";
>> @@ -1281,10 +1293,18 @@ for (i = 0; i < n_opt_val; i++) {
>>              print "  if (ptr->" name")";
>>              print "    ptr->" name" = xstrdup (ptr->" name");";
>>      }
>> -    else if (otype ~ "^unsigned")
>> -            print "  ptr->" name" = (" var_opt_val_type[i] ") bp_unpack_var_len_unsigned (bp);";
>> -    else
>> -            print "  ptr->" name" = (" var_opt_val_type[i] ") bp_unpack_var_len_int (bp);";
>> +    else {
>> +            if (otype ~ "^unsigned") {
>> +                    sgn = "unsigned";
>> +            } else {
>> +                    sgn = "int";
>> +            }
>> +            print "  ptr->" name" = (" var_opt_val_type[i] ") bp_unpack_var_len_" sgn " (bp);";
>> +            if (name ~ "^x_param" && !(otype ~ "^enum ") && var_opt_init[i]) {
>> +                    print "  if (" var_opt_init[i] " > (" var_opt_val_type[i] ") 10)";
>> +                    print "    ptr->" name" ^= " var_opt_init[i] ";";
>> +            }
>> +    }
>>  }
>>  print "  for (size_t i = 0; i < sizeof (ptr->explicit_mask) / sizeof (ptr->explicit_mask[0]); i++)";
>>  print "    ptr->explicit_mask[i] = bp_unpack_value (bp, 64);";


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
  

Comments

Joseph Myers Oct. 26, 2022, 6:21 p.m. UTC | #1
On Wed, 26 Oct 2022, Thomas Schwinge wrote:

> Hi!
> 
> Ping.  For convenience, I've re-attached
> "options: Clarify 'Init' option property usage for streaming optimization".
> (I've re-verified: "No functional change; no change in generated files.")

OK.
  

Patch

From c7968fa44b1073dfa7da3a11470a9e76b6faafaf Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
Date: Thu, 31 Mar 2022 12:06:29 +0200
Subject: [PATCH] options: Clarify 'Init' option property usage for streaming
 optimization

This clarifies commit 95db7e9afe57ca1c269d46baa2accced004e5c74
"options, lto: Optimize streaming of optimization nodes".

No functional change; no change in generated files.

	gcc/
	* optc-save-gen.awk: Clarify 'Init' option property usage for
	streaming optimization.
---
 gcc/optc-save-gen.awk | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/gcc/optc-save-gen.awk b/gcc/optc-save-gen.awk
index 9911bab6668..c60b300f693 100644
--- a/gcc/optc-save-gen.awk
+++ b/gcc/optc-save-gen.awk
@@ -1292,7 +1292,22 @@  for (i = 0; i < n_opts; i++) {
 		var_opt_val_type[n_opt_val] = otype;
 		var_opt_val[n_opt_val] = "x_" name;
 		var_opt_hash[n_opt_val] = flag_set_p("Optimization", flags[i]);
-		var_opt_init[n_opt_val] = opt_args("Init", flags[i]);
+
+		# If applicable, optimize streaming for the common case that
+		# the current value is unchanged from the 'Init' value:
+		# XOR-encode it so that we stream value zero.
+		# Not handling non-parameters as those really generally don't
+		# have large initializers.
+		# Not handling enums as we don't know if '(enum ...) 10' is
+		# even valid (see synthesized 'if' conditionals below).
+		if (flag_set_p("Param", flags[i]) \
+		    && !(otype ~ "^enum ")) {
+			# Those without 'Init' are zero-initialized and thus
+			# already encoded ideally.
+			init = opt_args("Init", flags[i])
+			var_opt_optimize_init[n_opt_val] = init;
+		}
+
 		n_opt_val++;
 	}
 }
@@ -1370,9 +1385,10 @@  for (i = 0; i < n_opt_val; i++) {
 		} else {
 			sgn = "int";
 		}
-		if (name ~ "^x_param" && !(otype ~ "^enum ") && var_opt_init[i]) {
-			print "  if (" var_opt_init[i] " > (" var_opt_val_type[i] ") 10)";
-			print "    bp_pack_var_len_" sgn " (bp, ptr->" name" ^ " var_opt_init[i] ");";
+		# If applicable, encode the streamed value.
+		if (var_opt_optimize_init[i]) {
+			print "  if (" var_opt_optimize_init[i] " > (" var_opt_val_type[i] ") 10)";
+			print "    bp_pack_var_len_" sgn " (bp, ptr->" name" ^ " var_opt_optimize_init[i] ");";
 			print "  else";
 			print "    bp_pack_var_len_" sgn " (bp, ptr->" name");";
 		} else {
@@ -1406,9 +1422,10 @@  for (i = 0; i < n_opt_val; i++) {
 			sgn = "int";
 		}
 		print "  ptr->" name" = (" var_opt_val_type[i] ") bp_unpack_var_len_" sgn " (bp);";
-		if (name ~ "^x_param" && !(otype ~ "^enum ") && var_opt_init[i]) {
-			print "  if (" var_opt_init[i] " > (" var_opt_val_type[i] ") 10)";
-			print "    ptr->" name" ^= " var_opt_init[i] ";";
+		# If applicable, decode the streamed value.
+		if (var_opt_optimize_init[i]) {
+			print "  if (" var_opt_optimize_init[i] " > (" var_opt_val_type[i] ") 10)";
+			print "    ptr->" name" ^= " var_opt_optimize_init[i] ";";
 		}
 	}
 }
-- 
2.35.1