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

2017-10-08 Thread Eric Botcazou 
> I'll take your word for it ;)

Thanks.  Testing on PowerPC64 revealed a couple of nits:

 1. SSA names with zero uses need to be excluded from the computation, because 
the first SSA name in a function returning a GIMPLE type is associated with 
the RESULT_DECL and is undefined, so it would propagate the undefinedness to 
every SSA name collapsed with the RESULT_DECL, i.e. unduly pessimization.

 2. Removing the SUBREG_PROMOTED_VAR_P flag disables the trick present in 
expand_gimple_stmt_1 for the LHS of assignment statements, so you end up with 
(more) SUBREGs on the LHS of moves in RTL, hence the fixlet for loop-iv.c.

I bootstrapped/regtested it on x86-64, SPARC64, Aarch64 and PowerPC64/Linux, 
and compared the code generated for the tests in gcc.c-torture/compile at -O2: 
very few changes for the first 3, but around 30 tests changed for PowerPC64, 
all with uninitialized variables AFAICS.

Applied on the mainline.


2017-10-08  Eric Botcazou  

* 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) : For a SSA_NAME, do
not set SUBREG_PROMOTED_VAR_P on the sub-register if it may contain
uninitialized bits.
* loop-iv.c (iv_get_reaching_def): Disqualify all subregs.


2017-10-08  Eric Botcazou  

* gcc.c-torture/execute/20171008-1.c: New test.

-- 
Eric BotcazouIndex: expr.c
===
--- expr.c	(revision 253506)
+++ 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, );
+	{
+	  pmode = promote_decl_mode (exp, );
+	  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, ,
-	   gimple_call_fntype (g),
-	   2);
+	{
+	  pmode = promote_function_mode (type, mode, ,
+	gimple_call_fntype (g), 2);
+	  always_initialized_rtx
+		= always_initialized_rtx_for_ssa_name_p (ssa_name);
+	}
 	  else
-	pmode = promote_ssa_mode (ssa_name, );
+	{
+	  pmode = promote_ssa_mode (ssa_name, );
+	  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: loop-iv.c
===
--- loop-iv.c	(revision 253506)
+++ loop-iv.c	(working copy)
@@ -353,7 +353,7 @@ iv_get_reaching_def (rtx_insn *insn, rtx
   adef = DF_REF_CHAIN (use)->ref;
 
   /* We do not handle setting only part of the register.  */
-  if (DF_REF_FLAGS (adef) & DF_REF_READ_WRITE)
+  if (DF_REF_FLAGS (adef) & (DF_REF_READ_WRITE | DF_REF_SUBREG))
 return GRD_INVALID;
 
   def_insn = DF_REF_INSN (adef);
Index: tree-outof-ssa.c
===
--- tree-outof-ssa.c	(revision 253506)
+++ 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 253506)
+++ tree-outof-ssa.h	(working copy)
@@ -42,6 +42,10 @@ struct ssaexpand
   /* If partition I contains an SSA name that has a default def

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

2017-10-05 Thread Richard Biener 
On Tue, Oct 3, 2017 at 8:39 PM, Eric Botcazou  wrote:
>> 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.

I'll take your word for it ;)

The patch is ok.

Thanks,
Richard.

>
> * 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) : For a SSA_NAME, do
> not set SUBREG_PROMOTED_VAR_P on the sub-register if it may contain
> uninitialized bits.
>
> --
> Eric Botcazou

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

2017-10-03 Thread Eric Botcazou 
> 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) : For a SSA_NAME, do
not set SUBREG_PROMOTED_VAR_P on the sub-register if it may contain
uninitialized bits.

-- 
Eric BotcazouIndex: 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, );
+	{
+	  pmode = promote_decl_mode (exp, );
+	  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, ,
-	   gimple_call_fntype (g),
-	   2);
+	{
+	  pmode = promote_function_mode (type, mode, ,
+	gimple_call_fntype (g), 2);
+	  always_initialized_rtx
+		= always_initialized_rtx_for_ssa_name_p (ssa_name);
+	}
 	  else
-	pmode = promote_ssa_mode (ssa_name, );
+	{
+	  pmode = promote_ssa_mode (ssa_name, );
+	  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

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

2017-09-25 Thread Richard Biener 
On Mon, Sep 25, 2017 at 11:35 AM, Eric Botcazou  wrote:
>> ISTR SUBREG_PROMOTED_* gets set by RTL expansion, the problematic
>> one should be the one in expr.c setting it on all SSA_NAMEs, maybe it is
>> enough to avoid setting it for SSA_NAME_IS_DEFAULT_DEF ones?
>
> No, it's not enough in this case, you need to work a little harder and look at
> the arguments of a PHI node because the SSA_NAME_IS_DEFAULT_DEF name is never
> materialized in the RTL; so the attached patch works in this case.
>
>
> * expr.c (expand_expr_real_1) : For a SSA_NAME, do
> not set SUBREG_PROMOTED_VAR_P on the sub-register if it may contain
> uninitialized bits.

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.

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).

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).

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

I don't stand in the way of "improving" things with your patch (besides the
ssa_undefined_value_p use).

Thanks,
Richard.

> --
> Eric Botcazou

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

2017-09-25 Thread Eric Botcazou 
> ISTR SUBREG_PROMOTED_* gets set by RTL expansion, the problematic
> one should be the one in expr.c setting it on all SSA_NAMEs, maybe it is
> enough to avoid setting it for SSA_NAME_IS_DEFAULT_DEF ones?

No, it's not enough in this case, you need to work a little harder and look at 
the arguments of a PHI node because the SSA_NAME_IS_DEFAULT_DEF name is never 
materialized in the RTL; so the attached patch works in this case.


* expr.c (expand_expr_real_1) : For a SSA_NAME, do
not set SUBREG_PROMOTED_VAR_P on the sub-register if it may contain
uninitialized bits.

-- 
Eric BotcazouIndex: expr.c
===
--- expr.c	(revision 253114)
+++ expr.c	(working copy)
@@ -9908,6 +9908,7 @@ expand_expr_real_1 (tree exp, rtx target
 	  && dmode != BLKmode
 	  && GET_MODE (decl_rtl) != dmode)
 	{
+	  bool maybe_uninitialized = false;
 	  machine_mode pmode;
 
 	  /* Get the signedness to be used for this variable.  Ensure we get
@@ -9918,15 +9919,42 @@ expand_expr_real_1 (tree exp, rtx target
 		   && gimple_code (g) == GIMPLE_CALL
 		   && !gimple_call_internal_p (g))
 	pmode = promote_function_mode (type, mode, ,
-	   gimple_call_fntype (g),
-	   2);
+	   gimple_call_fntype (g), 2);
 	  else
-	pmode = promote_ssa_mode (ssa_name, );
+	{
+	  use_operand_p arg_p;
+	  ssa_op_iter i;
+
+	  /* Try to detect if the register contains uninitialized bits.  */
+	  if (SSA_NAME_IS_DEFAULT_DEF (ssa_name))
+		maybe_uninitialized = true;
+	  else if (gimple_code (g) == GIMPLE_PHI)
+		FOR_EACH_PHI_ARG (arg_p, as_a (g), i, SSA_OP_USE)
+		  {
+		tree arg = USE_FROM_PTR (arg_p);
+		if (TREE_CODE (arg) == SSA_NAME
+			&& SSA_NAME_IS_DEFAULT_DEF (arg))
+		  {
+			maybe_uninitialized = true;
+			break;
+		  }
+		  }
+
+	  pmode = promote_ssa_mode (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 a previous extension if the
+	 register may contain uninitialized bits.  */
+	  if (!maybe_uninitialized)
+	{
+	  SUBREG_PROMOTED_VAR_P (temp) = 1;
+	  SUBREG_PROMOTED_SET (temp, unsignedp);
+	}
+
 	  return temp;
 	}

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

2017-09-13 Thread Richard Biener 
On Mon, Sep 11, 2017 at 9:59 PM, Eric Botcazou  wrote:
>> I think the issue is that we set SUBREG_PROMOTED_* on something that is
>> possibly not so (aka uninitialized in this case).
>
> Yes, that's what I called inherent weakness of the promoted subregs mechanism.
>
>> We may only set it if either the ABI or a previous operation forced it to.
>> Maybe this is also the reason why we need this zero init pass in some cases
>> (though that isn't a full solution either).
>
> Do you think that we should zero-init all the unsigned promoted subregs (and
> sign-extend-init all the signed promoted subregs)?  That sounds like a big
> hammer to me, but I can give it a try.

My suggestion would be to not set SUBREG_PROMOTED_* on "everything"
(which we seem to do).  zero-initing looks like the easier way to deal with
the situation though.

ISTR SUBREG_PROMOTED_* gets set by RTL expansion, the problematic
one should be the one in expr.c setting it on all SSA_NAMEs, maybe it is
enough to avoid setting it for SSA_NAME_IS_DEFAULT_DEF ones?

Richard.

> --
> Eric Botcazou

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

2017-09-11 Thread Eric Botcazou 
> I think the issue is that we set SUBREG_PROMOTED_* on something that is
> possibly not so (aka uninitialized in this case).

Yes, that's what I called inherent weakness of the promoted subregs mechanism.

> We may only set it if either the ABI or a previous operation forced it to.
> Maybe this is also the reason why we need this zero init pass in some cases
> (though that isn't a full solution either).

Do you think that we should zero-init all the unsigned promoted subregs (and 
sign-extend-init all the signed promoted subregs)?  That sounds like a big 
hammer to me, but I can give it a try.

-- 
Eric Botcazou

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

2017-09-11 Thread Richard Biener 
On September 11, 2017 12:26:09 PM GMT+02:00, Eric Botcazou 
 wrote:
>Hi,
>
>this is a bug originally reported in Ada on 32-bit PowerPC with the
>SVR4 ABI 
>(hence not Linux) and reproducible in C with the attached testcase at
>-O1.
>
>The problem lies in function 'foo':
>
>  struct S ret;
>  char r, s, c1, c2;
>  char *p = 
>
>  s = bar ();
>  if (s)
>c2 = *p;
>  c1 = 0;
>
>  ret.c1 = c1;
>  ret.c2 = c2;
>  return ret;
>
>Since the call to bar returns 0, c2 is uninitialized at run time (but
>this 
>doesn't matter for correctness since its value is never read).  Now
>both c1 
>and c2 are represented at the RTL level by unsigned promoted subregs,
>i.e. 
>SUBREGs verifying SUBREG_PROMOTED_VAR_P and SUBREG_PROMOTED_UNSIGNED_P
>
>As a consequence, when store_fixed_bit_field_1 stores the 8-bit values
>into 
>the 'ret' object, it considers that the underlying 32-bit objects have
>only 8 
>bits set and the rest cleared so it doesn't mask the other 24 bits. 
>That's 
>OK for c1 but not for c2, since c2 is uninitialized so the bits are
>random.
>
>This appears to be an inherent weakness of the promoted subregs
>mechanism, but 
>I don't think that we want to go for an upheaval at this point.  So the
>patch 
>addresses the problem in an ad-hoc manner directly in
>store_fixed_bit_field_1 
>and yields the expected 8-bit insertion:
>
>@@ -26,7 +26,7 @@
>lwz 9,12(1)
>lbz 31,0(9)
> .L3:
>-   slwi 3,31,16
>+   rlwinm 3,31,16,8,15
>lwz 0,36(1)
>mtlr 0
>lwz 31,28(1)
>
>Tested on x86-64/Linux and PowerPC64/Linux, OK for the mainline?

I think the issue is that we set SUBREG_PROMOTED_* on something that is 
possibly not so (aka uninitialized in this case). We may only set it if either 
the ABI or a previous operation forced it to. Maybe this is also the reason why 
we need this zero init pass in some cases (though that isn't a full solution 
either). 

Richard. 

>
>2017-09-11  Eric Botcazou  
>
>   * expmed.c (store_fixed_bit_field_1): Force the masking if the value
>   is an unsigned promoted SUBREG of a sufficiently large object.
>
>
>2017-09-11  Eric Botcazou  
>
>   * gcc.c-torture/execute/20170911-1.c: New test.

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

2017-09-11 Thread Eric Botcazou 
> this is a bug originally reported in Ada on 32-bit PowerPC with the SVR4 ABI
> (hence not Linux) and reproducible in C with the attached testcase at -O1.

With the right C testcase this time...

-- 
Eric Botcazoustruct S { char c1, c2, c3, c4; } __attribute__((aligned(4)));

static char bar (char **p) __attribute__((noclone, noinline));
static struct S foo (void) __attribute__((noclone, noinline));

int i;

static char
bar (char **p)
{
  i = 1;
  return 0;
}

static struct S
foo (void)
{
  struct S ret;
  char r, s, c1, c2;
  char *p = 

  s = bar ();
  if (s)
c2 = *p;
  c1 = 0;

  ret.c1 = c1;
  ret.c2 = c2;
  return ret;
}

int main (void)
{
  struct S s = foo ();
  if (s.c1 != 0)
__builtin_abort ();
  return 0;
}

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

2017-09-11 Thread Eric Botcazou 
Hi,

this is a bug originally reported in Ada on 32-bit PowerPC with the SVR4 ABI 
(hence not Linux) and reproducible in C with the attached testcase at -O1.

The problem lies in function 'foo':

  struct S ret;
  char r, s, c1, c2;
  char *p = 

  s = bar ();
  if (s)
c2 = *p;
  c1 = 0;

  ret.c1 = c1;
  ret.c2 = c2;
  return ret;

Since the call to bar returns 0, c2 is uninitialized at run time (but this 
doesn't matter for correctness since its value is never read).  Now both c1 
and c2 are represented at the RTL level by unsigned promoted subregs, i.e. 
SUBREGs verifying SUBREG_PROMOTED_VAR_P and SUBREG_PROMOTED_UNSIGNED_P

As a consequence, when store_fixed_bit_field_1 stores the 8-bit values into 
the 'ret' object, it considers that the underlying 32-bit objects have only 8 
bits set and the rest cleared so it doesn't mask the other 24 bits.  That's 
OK for c1 but not for c2, since c2 is uninitialized so the bits are random.

This appears to be an inherent weakness of the promoted subregs mechanism, but 
I don't think that we want to go for an upheaval at this point.  So the patch 
addresses the problem in an ad-hoc manner directly in store_fixed_bit_field_1 
and yields the expected 8-bit insertion:

@@ -26,7 +26,7 @@
lwz 9,12(1)
lbz 31,0(9)
 .L3:
-   slwi 3,31,16
+   rlwinm 3,31,16,8,15
lwz 0,36(1)
mtlr 0
lwz 31,28(1)

Tested on x86-64/Linux and PowerPC64/Linux, OK for the mainline?


2017-09-11  Eric Botcazou  

* expmed.c (store_fixed_bit_field_1): Force the masking if the value
is an unsigned promoted SUBREG of a sufficiently large object.


2017-09-11  Eric Botcazou  

* gcc.c-torture/execute/20170911-1.c: New test.

-- 
Eric BotcazouIndex: expmed.c
===
--- expmed.c	(revision 251906)
+++ expmed.c	(working copy)
@@ -1213,13 +1213,25 @@ store_fixed_bit_field_1 (rtx op0, scalar
 }
   else
 {
-  int must_and = (GET_MODE_BITSIZE (value_mode) != bitsize
-		  && bitnum + bitsize != GET_MODE_BITSIZE (mode));
+  /* Compute whether we must mask the value in order to make sure that the
+	 bits outside the bitfield are all cleared.  Note that, even though the
+	 value has the right size, if it has a mode different from MODE, then
+	 converting it to MODE may unmask uninitialized bits in the case where
+	 it is an unsigned promoted SUBREG of a sufficiently large object.  */
+  const bool must_mask
+	= (GET_MODE_BITSIZE (value_mode) != bitsize
+	   || (value_mode != mode
+	   && GET_CODE (value) == SUBREG
+	   && SUBREG_PROMOTED_VAR_P (value)
+	   && SUBREG_PROMOTED_UNSIGNED_P (value)
+	   && GET_MODE_SIZE (GET_MODE (SUBREG_REG (value)))
+		  >= GET_MODE_SIZE (mode)))
+	  && bitnum + bitsize != GET_MODE_BITSIZE (mode);
 
   if (value_mode != mode)
 	value = convert_to_mode (mode, value, 1);
 
-  if (must_and)
+  if (must_mask)
 	value = expand_binop (mode, and_optab, value,
 			  mask_rtx (mode, 0, bitsize, 0),
 			  NULL_RTX, 1, OPTAB_LIB_WIDEN);
unsigned long foo (int x)
{
  return x > 0 ? (unsigned long) x : 0;
}

unsigned long bar (int x, int y)
{
  return x > y ? (unsigned long) x : (unsigned long) y;
}