[patch] Fix PR middle-end/68291 & 68292

[patch] Fix PR middle-end/68291 & 68292

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

Hi,

it's a couple of regressions in the C testsuite present on SPARC 64-bit and 
coming from the new coalescing code which fails to handle vector types with 
BLKmode that are returned in multiple registers.  The code assigns a BLKmode 
REG to the RESULT_DECL of the function in expand_function_start and this later 
causes expand_function_end to choke.

As discussed with Alexandre in the audit trail, the attached minimal fix just 
prevents the problematic BLKmode REG from being generated, which appears to be 
sufficient to restore the nominal operating mode.

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


2015-12-07  Eric Botcazou  <ebotcazou@adacore.com>

	PR middle-end/68291
	PR middle-end/68292
	* cfgexpand.c (set_rtl): Always accept PARALLELs with BLKmode for
	SSA names based on RESULT_DECLs.
	* function.c (expand_function_start): Do not create BLKmode REGs
	for GIMPLE registers when coalescing is enabled.

-- 
Eric Botcazou

[-- Attachment #2: pr68291.diff --]
[-- Type: text/x-patch, Size: 2094 bytes --]

Index: cfgexpand.c
===================================================================
--- cfgexpand.c	(revision 231318)
+++ cfgexpand.c	(working copy)
@@ -184,10 +184,15 @@ set_rtl (tree t, rtx x)
 				      || SUBREG_P (XEXP (x, 0)))
 				  && (REG_P (XEXP (x, 1))
 				      || SUBREG_P (XEXP (x, 1))))
+			      /* We need to accept PARALLELs for RESUT_DECLs
+				 because of vector types with BLKmode returned
+				 in multiple registers, but they are supposed
+				 to be uncoalesced.  */
 			      || (GET_CODE (x) == PARALLEL
 				  && SSAVAR (t)
 				  && TREE_CODE (SSAVAR (t)) == RESULT_DECL
-				  && !flag_tree_coalesce_vars))
+				  && (GET_MODE (x) == BLKmode
+				      || !flag_tree_coalesce_vars)))
 			   : (MEM_P (x) || x == pc_rtx
 			      || (GET_CODE (x) == CONCAT
 				  && MEM_P (XEXP (x, 0))
Index: function.c
===================================================================
--- function.c	(revision 231318)
+++ function.c	(working copy)
@@ -5148,15 +5148,16 @@ expand_function_start (tree subr)
       /* Compute the return values into a pseudo reg, which we will copy
 	 into the true return register after the cleanups are done.  */
       tree return_type = TREE_TYPE (res);
-      /* If we may coalesce this result, make sure it has the expected
-	 mode.  */
-      if (flag_tree_coalesce_vars && is_gimple_reg (res))
-	{
-	  tree def = ssa_default_def (cfun, res);
-	  gcc_assert (def);
-	  machine_mode mode = promote_ssa_mode (def, NULL);
-	  set_parm_rtl (res, gen_reg_rtx (mode));
-	}
+
+      /* If we may coalesce this result, make sure it has the expected mode
+	 in case it was promoted.  But we need not bother about BLKmode.  */
+      machine_mode promoted_mode
+	= flag_tree_coalesce_vars && is_gimple_reg (res)
+	  ? promote_ssa_mode (ssa_default_def (cfun, res), NULL)
+	  : BLKmode;
+
+      if (promoted_mode != BLKmode)
+	set_parm_rtl (res, gen_reg_rtx (promoted_mode));
       else if (TYPE_MODE (return_type) != BLKmode
 	       && targetm.calls.return_in_msb (return_type))
 	/* expand_function_end will insert the appropriate padding in

Re: [patch] Fix PR middle-end/68291 & 68292

[Bernd Schmidt]

On 12/07/2015 10:35 AM, Eric Botcazou wrote:
> As discussed with Alexandre in the audit trail, the attached minimal fix just
> prevents the problematic BLKmode REG from being generated, which appears to be
> sufficient to restore the nominal operating mode.

>
> 	PR middle-end/68291
> 	PR middle-end/68292
> 	* cfgexpand.c (set_rtl): Always accept PARALLELs with BLKmode for
> 	SSA names based on RESULT_DECLs.
> 	* function.c (expand_function_start): Do not create BLKmode REGs
> 	for GIMPLE registers when coalescing is enabled.
>

Ok. Although thinking about your comment in the PR about not making such 
vectors gimple registers I wonder what the effects of that would be.


Bernd

Re: [patch] Fix PR middle-end/68291 & 68292

[Eric Botcazou]

> Ok. Although thinking about your comment in the PR about not making such
> vectors gimple registers I wonder what the effects of that would be.

First of all it's a bit painful to do because is_gimple_reg_type is defined 
inline in gimple-expr.h and adding TYPE_MODE in there causes a compilation 
failure for a bunch of files.  More seriously, this can probably be seen as a 
real layering violation so I'm not sure this would be a progress.

-- 
Eric Botcazou

Re: [patch] Fix PR middle-end/68291 & 68292

[Richard Biener]

On December 7, 2015 5:42:02 PM GMT+01:00, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Ok. Although thinking about your comment in the PR about not making
>such
>> vectors gimple registers I wonder what the effects of that would be.
>
>First of all it's a bit painful to do because is_gimple_reg_type is
>defined 
>inline in gimple-expr.h and adding TYPE_MODE in there causes a
>compilation 
>failure for a bunch of files.  More seriously, this can probably be
>seen as a 
>real layering violation so I'm not sure this would be a progress.

Yeah, it would also have quite some impact on optimization.  Note that if only specific decls are involved they can be made non-registers via DECL_GIMPLE_REG_P.

Richard.

Re: [patch] Fix PR middle-end/68291 & 68292

[Christophe Lyon]

On 7 December 2015 at 10:35, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> it's a couple of regressions in the C testsuite present on SPARC 64-bit and
> coming from the new coalescing code which fails to handle vector types with
> BLKmode that are returned in multiple registers.  The code assigns a BLKmode
> REG to the RESULT_DECL of the function in expand_function_start and this later
> causes expand_function_end to choke.
>
> As discussed with Alexandre in the audit trail, the attached minimal fix just
> prevents the problematic BLKmode REG from being generated, which appears to be
> sufficient to restore the nominal operating mode.
>
> Tested on x86-64/Linux and SPARC64/Solaris, OK for the mainline?
>
>
> 2015-12-07  Eric Botcazou  <ebotcazou@adacore.com>
>
>         PR middle-end/68291
>         PR middle-end/68292
>         * cfgexpand.c (set_rtl): Always accept PARALLELs with BLKmode for
>         SSA names based on RESULT_DECLs.
>         * function.c (expand_function_start): Do not create BLKmode REGs
>         for GIMPLE registers when coalescing is enabled.
>

Hi Eric,

Since you committed this (r231372), I've noticed several regressions
on ARM and AArch64.
You can have a look at
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/231372/report-build-info.html
for more details.

Can you have a look?

Thanks,

Christophe.


> --
> Eric Botcazou

Re: [patch] Fix PR middle-end/68291 & 68292

[Eric Botcazou]

> Since you committed this (r231372), I've noticed several regressions
> on ARM and AArch64.
> You can have a look at
> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/231372/
> report-build-info.html for more details.

I presume it's:

Fail appears              [     => FAIL]:
  gcc.dg/pr63594-1.c (internal compiler error)
  gcc.dg/pr63594-2.c (internal compiler error)
  gcc.dg/vect/vect-singleton_1.c (internal compiler error)
  gcc.dg/vect/vect-singleton_1.c -flto -ffat-lto-objects (internal compiler 
error)
  gcc.target/aarch64/aapcs64/func-ret-1.c compilation,  -O1  (internal 
compiler error)
  gcc.target/aarch64/aapcs64/func-ret-1.c compilation,  -O2  (internal 
compiler error)
  gcc.target/aarch64/aapcs64/func-ret-1.c compilation,  -O3 -g  (internal 
compiler error)
  gcc.target/aarch64/aapcs64/func-ret-1.c compilation,  -Og -g  (internal 
compiler error)
  gcc.target/aarch64/aapcs64/func-ret-1.c compilation,  -Os  (internal 
compiler error)
  gcc.target/aarch64/pr65491_1.c (internal compiler error)

on aarch64?  Yes, I'm going to have a look.

-- 
Eric Botcazou

Re: [patch] Fix PR middle-end/68291 & 68292

[Christophe Lyon]

On 8 December 2015 at 10:46, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Since you committed this (r231372), I've noticed several regressions
>> on ARM and AArch64.
>> You can have a look at
>> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/231372/
>> report-build-info.html for more details.
>
> I presume it's:
>
> Fail appears              [     => FAIL]:
>   gcc.dg/pr63594-1.c (internal compiler error)
>   gcc.dg/pr63594-2.c (internal compiler error)
>   gcc.dg/vect/vect-singleton_1.c (internal compiler error)
>   gcc.dg/vect/vect-singleton_1.c -flto -ffat-lto-objects (internal compiler
> error)
>   gcc.target/aarch64/aapcs64/func-ret-1.c compilation,  -O1  (internal
> compiler error)
>   gcc.target/aarch64/aapcs64/func-ret-1.c compilation,  -O2  (internal
> compiler error)
>   gcc.target/aarch64/aapcs64/func-ret-1.c compilation,  -O3 -g  (internal
> compiler error)
>   gcc.target/aarch64/aapcs64/func-ret-1.c compilation,  -Og -g  (internal
> compiler error)
>   gcc.target/aarch64/aapcs64/func-ret-1.c compilation,  -Os  (internal
> compiler error)
>   gcc.target/aarch64/pr65491_1.c (internal compiler error)
>
> on aarch64?  Yes, I'm going to have a look.
>

Yes you are right. the PASS->FAIL and "PASS disappears" are consequences
of the new failures above.
You can download the gcc.log files by clicking on the "log" link, it that helps.

Thanks,

Christophe.

> --
> Eric Botcazou

Re: [patch] Fix PR middle-end/68291 & 68292

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]

> Yes you are right. the PASS->FAIL and "PASS disappears" are consequences
> of the new failures above.

OK.  The issue is that we used to create a REG:BLK for RESULT_DECL, but now we 
get to hard_function_value as originally, which rightfully adjusts it to SI:

  val
   = targetm.calls.function_value (valtype, func ? func : fntype, outgoing);

  if (REG_P (val)
      && GET_MODE (val) == BLKmode)
    {
      unsigned HOST_WIDE_INT bytes = int_size_in_bytes (valtype);
      machine_mode tmpmode;

      /* int_size_in_bytes can return -1.  We don't need a check here
	 since the value of bytes will then be large enough that no
	 mode will match anyway.  */

      for (tmpmode = GET_CLASS_NARROWEST_MODE (MODE_INT);
	   tmpmode != VOIDmode;
	   tmpmode = GET_MODE_WIDER_MODE (tmpmode))
	{
	  /* Have we found a large enough mode?  */
	  if (GET_MODE_SIZE (tmpmode) >= bytes)
	    break;
	}

      /* No suitable mode found.  */
      gcc_assert (tmpmode != VOIDmode);

      PUT_MODE (val, tmpmode);
    }

and we assert in the second gcc_checking_assert of set_rtl because mode and 
promote_ssa_mode don't agree anymore (SI vs BLK).  So we need to relax the 
second gcc_checking_assert the same way as the first one.

I'm going to test it on x86-64, SPARC64 and Aarch64.


	PR middle-end/68291
	PR middle-end/68292
	* cfgexpand.c (set_rtl): Always accept mode mismatch for SSA names
	with BLKmode promoted mode based on RESULT_DECLs.


-- 
Eric Botcazou

[-- Attachment #2: pr68291-2.diff --]
[-- Type: text/x-patch, Size: 1144 bytes --]

Index: cfgexpand.c
===================================================================
--- cfgexpand.c	(revision 231394)
+++ cfgexpand.c	(working copy)
@@ -203,11 +203,14 @@ set_rtl (tree t, rtx x)
      PARM_DECLs and RESULT_DECLs, we'll have been called by
      set_parm_rtl, which will give us the default def, so we don't
      have to compute it ourselves.  For RESULT_DECLs, we accept mode
-     mismatches too, as long as we're not coalescing across variables,
-     so that we don't reject BLKmode PARALLELs or unpromoted REGs.  */
+     mismatches too, as long as we have BLKmode or are not coalescing
+     across variables, so that we don't reject BLKmode PARALLELs or
+     unpromoted REGs.  */
   gcc_checking_assert (!x || x == pc_rtx || TREE_CODE (t) != SSA_NAME
-		       || (SSAVAR (t) && TREE_CODE (SSAVAR (t)) == RESULT_DECL
-			   && !flag_tree_coalesce_vars)
+		       || (SSAVAR (t)
+			   && TREE_CODE (SSAVAR (t)) == RESULT_DECL
+			   && (promote_ssa_mode (t, NULL) == BLKmode
+			       || !flag_tree_coalesce_vars))
 		       || !use_register_for_decl (t)
 		       || GET_MODE (x) == promote_ssa_mode (t, NULL));
 

Re: [patch] Fix PR middle-end/68291 & 68292

[Bernd Schmidt]

On 12/08/2015 11:50 AM, Eric Botcazou wrote:
>
> I'm going to test it on x86-64, SPARC64 and Aarch64.
>
>
> 	PR middle-end/68291
> 	PR middle-end/68292
> 	* cfgexpand.c (set_rtl): Always accept mode mismatch for SSA names
> 	with BLKmode promoted mode based on RESULT_DECLs.

I think that is ok if the testing passes.


Bernd

Re: [patch] Fix PR middle-end/68291 & 68292

[Eric Botcazou]

> I think that is ok if the testing passes.

Thanks.  It did on the 3 architectures so I have applied the patch.

-- 
Eric Botcazou

Re: [patch] Fix PR middle-end/68291 & 68292

[Christophe Lyon]

On 8 December 2015 at 19:48, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I think that is ok if the testing passes.
>
> Thanks.  It did on the 3 architectures so I have applied the patch.
>

Thanks, I confirm it also fixes the regressions I noticed on ARM.

Christophe.

> --
> Eric Botcazou