Re: [patch] Fix wrong code with small structure return on PowerPC

[Eric Botcazou <ebotcazou at adacore dot com>]

> Reading the patch I think that it gets conservativeness wrong -- shouldn't
> it be
> 
>   if (is_definitely_initialized)
>    {
>       SUBREG_PROMOTED_VAR_P (temp) = 1;
>       SUBREG_PROMOTED_SET (temp, unsignedp);
>    }
> 
> ?  Of course it's not easy to compute is_definitely_initialized
> conservatively in an ad-hoc way at this place.  It should be relatively
> straight-forward to do a conservative computation somewhere in cfgexpand.c
> by propagating across SSA edges and recording a flag on SSA names though. 
> I assume we can take load destinations as fully initialized (should extend
> properly) as well as call results (the ABI should extend, eventually we can
> query the target if it does), likewise for function arguments.

Yes, that's why the comment read "Try to detect if " and not "Detect if ".

> On your patch:
> 
> +             /* Try to detect if the register contains uninitialized bits. 
> */ +             if (SSA_NAME_IS_DEFAULT_DEF (ssa_name))
> +               maybe_uninitialized = true;
> 
> if you use ssa_undefined_value_p (ssa_name[, true]) you'd get function
> paramters not undefined (which is probably desired?).  Likewise
> partial initialized complex would get uninitialized (if passed , true).

Ah, yes, I overlooked that.

> Same inside the loop over PHI args though I wonder how pessimizing
> it would be to simply do
> 
>   if (ssa_undefined_value_p (ssa_name, true)
> 
>       || gimple_code (SSA_NAME_DEF_STMT (ssa_name)) == GIMPLE_PHI)
> 
>     maybe_uninitialized = true;
> 
> thus make all PHIs possibly uninitialized (your code wouldn't catch a
> chained PHI with undef arg).

Too big a hammer for such a very rare bug I think.

> As said, a better solution would be to do a definitely-initialized
> mini-propagation at RTL expansion time.

I'm not sure if we really need to propagate.  What about the attached patch?  
It computes at expansion time whether the partition the SSA name is a member 
of contains an undefined value and, if so, doesn't set the promoted bit for 
the SUBREG.  My gut feeling is that it's sufficient in practice.


	* tree-outof-ssa.h (ssaexpand): Add partitions_for_undefined_values.
	(always_initialized_rtx_for_ssa_name_p): New predicate.
	* tree-outof-ssa.c (remove_ssa_form): Initialize new field of SA.
	(finish_out_of_ssa): Free new field of SA.
	* tree-ssa-coalesce.h (get_undefined_value_partitions): Declare.
	* tree-ssa-coalesce.c: Include tree-ssa.h.
	(get_parm_default_def_partitions): Remove extern keyword.
	(get_undefined_value_partitions): New function.
	* expr.c (expand_expr_real_1) <expand_decl_rtl>: For a SSA_NAME, do
	not set SUBREG_PROMOTED_VAR_P on the sub-register if it may contain
	uninitialized bits.

-- 
Eric Botcazou

Index: expr.c
===================================================================
--- expr.c	(revision 253377)
+++ expr.c	(working copy)
@@ -9909,24 +9909,43 @@ expand_expr_real_1 (tree exp, rtx target
 	  && GET_MODE (decl_rtl) != dmode)
 	{
 	  machine_mode pmode;
+	  bool always_initialized_rtx;
 
 	  /* Get the signedness to be used for this variable.  Ensure we get
 	     the same mode we got when the variable was declared.  */
 	  if (code != SSA_NAME)
-	    pmode = promote_decl_mode (exp, &unsignedp);
+	    {
+	      pmode = promote_decl_mode (exp, &unsignedp);
+	      always_initialized_rtx = true;
+	    }
 	  else if ((g = SSA_NAME_DEF_STMT (ssa_name))
 		   && gimple_code (g) == GIMPLE_CALL
 		   && !gimple_call_internal_p (g))
-	    pmode = promote_function_mode (type, mode, &unsignedp,
-					   gimple_call_fntype (g),
-					   2);
+	    {
+	      pmode = promote_function_mode (type, mode, &unsignedp,
+					    gimple_call_fntype (g), 2);
+	      always_initialized_rtx
+		= always_initialized_rtx_for_ssa_name_p (ssa_name);
+	    }
 	  else
-	    pmode = promote_ssa_mode (ssa_name, &unsignedp);
+	    {
+	      pmode = promote_ssa_mode (ssa_name, &unsignedp);
+	      always_initialized_rtx
+		= always_initialized_rtx_for_ssa_name_p (ssa_name);
+	    }
+
 	  gcc_assert (GET_MODE (decl_rtl) == pmode);
 
 	  temp = gen_lowpart_SUBREG (mode, decl_rtl);
-	  SUBREG_PROMOTED_VAR_P (temp) = 1;
-	  SUBREG_PROMOTED_SET (temp, unsignedp);
+
+	  /* We cannot assume anything about an existing extension if the
+	     register may contain uninitialized bits.  */
+	  if (always_initialized_rtx)
+	    {
+	      SUBREG_PROMOTED_VAR_P (temp) = 1;
+	      SUBREG_PROMOTED_SET (temp, unsignedp);
+	    }
+
 	  return temp;
 	}
 
Index: tree-outof-ssa.c
===================================================================
--- tree-outof-ssa.c	(revision 253377)
+++ tree-outof-ssa.c	(working copy)
@@ -969,6 +969,7 @@ remove_ssa_form (bool perform_ter, struc
   sa->map = map;
   sa->values = values;
   sa->partitions_for_parm_default_defs = get_parm_default_def_partitions (map);
+  sa->partitions_for_undefined_values = get_undefined_value_partitions (map);
 }
 
 
@@ -1144,6 +1145,7 @@ finish_out_of_ssa (struct ssaexpand *sa)
     BITMAP_FREE (sa->values);
   delete_var_map (sa->map);
   BITMAP_FREE (sa->partitions_for_parm_default_defs);
+  BITMAP_FREE (sa->partitions_for_undefined_values);
   memset (sa, 0, sizeof *sa);
 }
 
Index: tree-outof-ssa.h
===================================================================
--- tree-outof-ssa.h	(revision 253377)
+++ tree-outof-ssa.h	(working copy)
@@ -42,6 +42,10 @@ struct ssaexpand
   /* If partition I contains an SSA name that has a default def for a
      parameter, bit I will be set in this bitmap.  */
   bitmap partitions_for_parm_default_defs;
+
+  /* If partition I contains an SSA name that has an undefined value,
+     bit I will be set in this bitmap.  */
+  bitmap partitions_for_undefined_values;
 };
 
 /* This is the singleton described above.  */
@@ -70,6 +74,18 @@ get_gimple_for_ssa_name (tree exp)
   return NULL;
 }
 
+/* Return whether the RTX expression representing the storage of the outof-SSA
+   partition that the SSA name EXP is a member of is always initialized.  */
+static inline bool
+always_initialized_rtx_for_ssa_name_p (tree exp)
+{
+  int p = partition_find (SA.map->var_partition, SSA_NAME_VERSION (exp));
+  if (SA.map->partition_to_view)
+    p = SA.map->partition_to_view[p];
+  gcc_assert (p != NO_PARTITION);
+  return !bitmap_bit_p (SA.partitions_for_undefined_values, p);
+}
+
 extern bool ssa_is_replaceable_p (gimple *stmt);
 extern void finish_out_of_ssa (struct ssaexpand *sa);
 extern unsigned int rewrite_out_of_ssa (struct ssaexpand *sa);
Index: tree-ssa-coalesce.c
===================================================================
--- tree-ssa-coalesce.c	(revision 253377)
+++ tree-ssa-coalesce.c	(working copy)
@@ -28,6 +28,7 @@ along with GCC; see the file COPYING3.
 #include "memmodel.h"
 #include "tm_p.h"
 #include "ssa.h"
+#include "tree-ssa.h"
 #include "tree-pretty-print.h"
 #include "diagnostic-core.h"
 #include "dumpfile.h"
@@ -1923,7 +1924,7 @@ set_parm_default_def_partition (tree var
 /* Allocate and return a bitmap that has a bit set for each partition
    that contains a default def for a parameter.  */
 
-extern bitmap
+bitmap
 get_parm_default_def_partitions (var_map map)
 {
   bitmap parm_default_def_parts = BITMAP_ALLOC (NULL);
@@ -1935,3 +1936,25 @@ get_parm_default_def_partitions (var_map
 
   return parm_default_def_parts;
 }
+
+/* Allocate and return a bitmap that has a bit set for each partition
+   that contains an undefined value.  */
+
+bitmap
+get_undefined_value_partitions (var_map map)
+{
+  bitmap undefined_value_parts = BITMAP_ALLOC (NULL);
+
+  for (unsigned int i = 1; i < num_ssa_names; i++)
+    {
+      tree var = ssa_name (i);
+      if (var && !virtual_operand_p (var) && ssa_undefined_value_p (var))
+	{
+	  const int p = var_to_partition (map, var);
+	  if (p != NO_PARTITION)
+	    bitmap_set_bit (undefined_value_parts, p);
+	}
+    }
+
+  return undefined_value_parts;
+}
Index: tree-ssa-coalesce.h
===================================================================
--- tree-ssa-coalesce.h	(revision 253377)
+++ tree-ssa-coalesce.h	(working copy)
@@ -23,5 +23,6 @@ along with GCC; see the file COPYING3.
 extern var_map coalesce_ssa_name (void);
 extern bool gimple_can_coalesce_p (tree, tree);
 extern bitmap get_parm_default_def_partitions (var_map);
+extern bitmap get_undefined_value_partitions (var_map);
 
 #endif /* GCC_TREE_SSA_COALESCE_H */