diff mbox

Avoid bugs like PR68273 to trigger

Message ID alpine.LSU.2.11.1602081100180.31122@t29.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Feb. 8, 2016, 10:02 a.m. UTC 
This makes it less likely (for example through the PRE path) to trigger
target bugs like PR68273 where targets use type alignment of call
arguments to decide on the ABI.

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.

IMHO targets should still be fixed.

Richard.

2016-02-08  Richard Biener  <rguenther@suse.de>
	Jeff Law  <law@redhat.com>

	PR target/68273
	* tree-ssanames.c (make_ssa_name_fn): Always use unqualified
	types for anonymous SSA names.

	* gcc.target/mips/pr68273.c: New testcase.

Comments

Eric Botcazou Feb. 8, 2016, 10:19 a.m. UTC | #1 
> This makes it less likely (for example through the PRE path) to trigger
> target bugs like PR68273 where targets use type alignment of call
> arguments to decide on the ABI.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.

Thanks.  I think that we can also avoid this issue by teaching SRA not to 
over-align scalar types, I don't see any point in doing that.  It's done in:

  misalign = (misalign + offset) & (align - 1);
  if (misalign != 0)
    align = (misalign & -misalign);
  if (align != TYPE_ALIGN (exp_type))
    exp_type = build_aligned_type (exp_type, align);

  mem_ref = fold_build2_loc (loc, MEM_REF, exp_type, base, off);
Richard Biener Feb. 8, 2016, 10:25 a.m. UTC | #2 
On Mon, 8 Feb 2016, Eric Botcazou wrote:

> > This makes it less likely (for example through the PRE path) to trigger
> > target bugs like PR68273 where targets use type alignment of call
> > arguments to decide on the ABI.
> > 
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
> 
> Thanks.  I think that we can also avoid this issue by teaching SRA not to 
> over-align scalar types, I don't see any point in doing that.  It's done in:
> 
>   misalign = (misalign + offset) & (align - 1);
>   if (misalign != 0)
>     align = (misalign & -misalign);
>   if (align != TYPE_ALIGN (exp_type))
>     exp_type = build_aligned_type (exp_type, align);
> 
>   mem_ref = fold_build2_loc (loc, MEM_REF, exp_type, base, off);

No, that's not over-aliging a salar type, that's preserving alignment
information on the memory reference.  What would need to be fixed
here is to see where that 'mem_ref' is assigned to a SSA name and
avoid making that SSA name have an over-aligned type.

Richard.
Eric Botcazou Feb. 8, 2016, 10:33 a.m. UTC | #3 
> No, that's not over-aliging a salar type, that's preserving alignment
> information on the memory reference.

What would we lose exactly by lowering the alignment to that of the type?
What's the point in knowing that a 32-bit integer is 64-bit aligned at the 
GIMPLE level?
Richard Biener Feb. 8, 2016, 10:44 a.m. UTC | #4 
On Mon, 8 Feb 2016, Eric Botcazou wrote:

> > No, that's not over-aliging a salar type, that's preserving alignment
> > information on the memory reference.
> 
> What would we lose exactly by lowering the alignment to that of the type?
> What's the point in knowing that a 32-bit integer is 64-bit aligned at the 
> GIMPLE level?

It helps vectorization.  This was specifically introduced to fix a
regression on powerpc.

See PR65310.

Richard.
Eric Botcazou Feb. 8, 2016, 11:02 a.m. UTC | #5 
> It helps vectorization.  This was specifically introduced to fix a
> regression on powerpc.
> 
> See PR65310.

Indeed, the SRA code was much better before that one liner. ;-)
Richard Biener Feb. 8, 2016, 11:14 a.m. UTC | #6 
On Mon, 8 Feb 2016, Eric Botcazou wrote:

> > It helps vectorization.  This was specifically introduced to fix a
> > regression on powerpc.
> > 
> > See PR65310.
> 
> Indeed, the SRA code was much better before that one liner. ;-)

Not sure what to make of this comment ;)  Did you see where we
create the SSA name with the overaligned type?

Richard.
Eric Botcazou Feb. 8, 2016, 11:52 a.m. UTC | #7 
> Not sure what to make of this comment ;)

I guess it was a variant of the usual can-of-worms-opening thing. :-)

> Did you see where we create the SSA name with the overaligned type?

In tree-ssa-pre.c:insert_into_preds_of_block:

(gdb) p debug_pre_expr(expr)
{mem_ref<0B>,yyvsp_1}@.MEM_2

(gdb) p debug_tree(type)
 <pointer_type 0x7ffff6d335e8
    type <record_type 0x7ffff6cea498 Tnode type_0 BLK
        size <integer_cst 0x7ffff6c43678 constant 0>
        unit size <integer_cst 0x7ffff6c43618 constant 0>
        align 8 symtab 0 alias set -1 canonical type 0x7ffff6cea3f0 context 
<translation_unit_decl 0x7ffff6d2e0f0 D.1453>
        pointer_to_this <pointer_type 0x7ffff6ceac78> chain <type_decl 
0x7ffff6c54a18 D.1410>>
    sizes-gimplified unsigned SI
    size <integer_cst 0x7ffff6c435e8 type <integer_type 0x7ffff6c3c150 
bitsizetype> constant 32>
    unit size <integer_cst 0x7ffff6c43600 type <integer_type 0x7ffff6c3c0a8 
sizetype> constant 4>
    align 64 symtab 0 alias set -1 canonical type 0x7ffff6cead20>

  /* Now build a phi for the new variable.  */
  temp = make_temp_ssa_name (type, NULL, "prephitmp");
Richard Biener Feb. 8, 2016, 12:06 p.m. UTC | #8 
On Mon, 8 Feb 2016, Eric Botcazou wrote:

> > Not sure what to make of this comment ;)
> 
> I guess it was a variant of the usual can-of-worms-opening thing. :-)
> 
> > Did you see where we create the SSA name with the overaligned type?
> 
> In tree-ssa-pre.c:insert_into_preds_of_block:
> 
> (gdb) p debug_pre_expr(expr)
> {mem_ref<0B>,yyvsp_1}@.MEM_2
> 
> (gdb) p debug_tree(type)
>  <pointer_type 0x7ffff6d335e8
>     type <record_type 0x7ffff6cea498 Tnode type_0 BLK
>         size <integer_cst 0x7ffff6c43678 constant 0>
>         unit size <integer_cst 0x7ffff6c43618 constant 0>
>         align 8 symtab 0 alias set -1 canonical type 0x7ffff6cea3f0 context 
> <translation_unit_decl 0x7ffff6d2e0f0 D.1453>
>         pointer_to_this <pointer_type 0x7ffff6ceac78> chain <type_decl 
> 0x7ffff6c54a18 D.1410>>
>     sizes-gimplified unsigned SI
>     size <integer_cst 0x7ffff6c435e8 type <integer_type 0x7ffff6c3c150 
> bitsizetype> constant 32>
>     unit size <integer_cst 0x7ffff6c43600 type <integer_type 0x7ffff6c3c0a8 
> sizetype> constant 4>
>     align 64 symtab 0 alias set -1 canonical type 0x7ffff6cead20>
> 
>   /* Now build a phi for the new variable.  */
>   temp = make_temp_ssa_name (type, NULL, "prephitmp");

Yes, that place I just fixed.  I mean for the SRA case.

Richard.
Eric Botcazou Feb. 8, 2016, 12:30 p.m. UTC | #9 
> Yes, that place I just fixed.  I mean for the SRA case.

Are you sure that there is one?

	  expr = build_ref_for_model (loc, agg, access->offset - top_offset,
				      access, gsi, insert_after);

	  if (write)
	    {
	      if (access->grp_partial_lhs)
		expr = force_gimple_operand_gsi (gsi, expr, true, NULL_TREE,
						 !insert_after,
						 insert_after ? GSI_NEW_STMT
						 : GSI_SAME_STMT);
	      stmt = gimple_build_assign (repl, expr);


(gdb) p debug_tree(repl)        
 <var_decl 0x7ffff6d37870 q$typ
    type <pointer_type 0x7ffff6ceac78
        type <record_type 0x7ffff6cea498 Tnode type_0 BLK
            size <integer_cst 0x7ffff6c43678 constant 0>
            unit size <integer_cst 0x7ffff6c43618 constant 0>
            align 8 symtab 0 alias set -1 canonical type 0x7ffff6cea3f0 
context <translation_unit_decl 0x7ffff6d2e0f0 D.1453>
            pointer_to_this <pointer_type 0x7ffff6ceac78> chain <type_decl 
0x7ffff6c54a18 D.1410>>
        sizes-gimplified unsigned SI
        size <integer_cst 0x7ffff6c435e8 constant 32>
        unit size <integer_cst 0x7ffff6c43600 constant 4>
        align 32 symtab 0 alias set -1 canonical type 0x7ffff6cead20>
    used unsigned SI file pr68273.c line 67 col 10 size <integer_cst 
0x7ffff6c435e8 32> unit size <integer_cst 0x7ffff6c43600 4>
    align 32 context <function_decl 0x7ffff6d0e700 op>>


(gdb) p debug_tree(expr)
 <mem_ref 0x7ffff6d31a78
    type <pointer_type 0x7ffff6d335e8
        type <record_type 0x7ffff6cea498 Tnode type_0 BLK
            size <integer_cst 0x7ffff6c43678 constant 0>
            unit size <integer_cst 0x7ffff6c43618 constant 0>
            align 8 symtab 0 alias set -1 canonical type 0x7ffff6cea3f0 
context <translation_unit_decl 0x7ffff6d2e0f0 D.1453>
            pointer_to_this <pointer_type 0x7ffff6ceac78> chain <type_decl 
0x7ffff6c54a18 D.1410>>
        sizes-gimplified unsigned SI
        size <integer_cst 0x7ffff6c435e8 constant 32>
        unit size <integer_cst 0x7ffff6c43600 constant 4>
        align 64 symtab 0 alias set -1 canonical type 0x7ffff6cead20>
   
    arg 0 <addr_expr 0x7ffff6d1d8a0
        type <pointer_type 0x7ffff6d33348 type <record_type 0x7ffff6ceadc8 
Node>
            unsigned SI size <integer_cst 0x7ffff6c435e8 32> unit size 
<integer_cst 0x7ffff6c43600 4>
            align 32 symtab 0 alias set -1 canonical type 0x7ffff6d333f0>
       
        arg 0 <parm_decl 0x7ffff6d2d300 q type <record_type 0x7ffff6ceadc8 
Node>
            used BLK file pr68273.c line 67 col 10
            size <integer_cst 0x7ffff6c43948 constant 128>
            unit size <integer_cst 0x7ffff6c43960 constant 16>
            align 64 context <function_decl 0x7ffff6d0e700 op> arg-type 
<record_type 0x7ffff6ceadc8 Node>>>
    arg 1 <integer_cst 0x7ffff6d1c4f8 type <pointer_type 0x7ffff6d333f0> 
constant 0>>
Richard Biener Feb. 8, 2016, 12:55 p.m. UTC | #10 
On Mon, 8 Feb 2016, Eric Botcazou wrote:

> > Yes, that place I just fixed.  I mean for the SRA case.
> 
> Are you sure that there is one?

No, but if there is none left why would you want to "fix" SRA?

Richard.

> 	  expr = build_ref_for_model (loc, agg, access->offset - top_offset,
> 				      access, gsi, insert_after);
> 
> 	  if (write)
> 	    {
> 	      if (access->grp_partial_lhs)
> 		expr = force_gimple_operand_gsi (gsi, expr, true, NULL_TREE,
> 						 !insert_after,
> 						 insert_after ? GSI_NEW_STMT
> 						 : GSI_SAME_STMT);
> 	      stmt = gimple_build_assign (repl, expr);
> 
> 
> (gdb) p debug_tree(repl)        
>  <var_decl 0x7ffff6d37870 q$typ
>     type <pointer_type 0x7ffff6ceac78
>         type <record_type 0x7ffff6cea498 Tnode type_0 BLK
>             size <integer_cst 0x7ffff6c43678 constant 0>
>             unit size <integer_cst 0x7ffff6c43618 constant 0>
>             align 8 symtab 0 alias set -1 canonical type 0x7ffff6cea3f0 
> context <translation_unit_decl 0x7ffff6d2e0f0 D.1453>
>             pointer_to_this <pointer_type 0x7ffff6ceac78> chain <type_decl 
> 0x7ffff6c54a18 D.1410>>
>         sizes-gimplified unsigned SI
>         size <integer_cst 0x7ffff6c435e8 constant 32>
>         unit size <integer_cst 0x7ffff6c43600 constant 4>
>         align 32 symtab 0 alias set -1 canonical type 0x7ffff6cead20>
>     used unsigned SI file pr68273.c line 67 col 10 size <integer_cst 
> 0x7ffff6c435e8 32> unit size <integer_cst 0x7ffff6c43600 4>
>     align 32 context <function_decl 0x7ffff6d0e700 op>>
> 
> 
> (gdb) p debug_tree(expr)
>  <mem_ref 0x7ffff6d31a78
>     type <pointer_type 0x7ffff6d335e8
>         type <record_type 0x7ffff6cea498 Tnode type_0 BLK
>             size <integer_cst 0x7ffff6c43678 constant 0>
>             unit size <integer_cst 0x7ffff6c43618 constant 0>
>             align 8 symtab 0 alias set -1 canonical type 0x7ffff6cea3f0 
> context <translation_unit_decl 0x7ffff6d2e0f0 D.1453>
>             pointer_to_this <pointer_type 0x7ffff6ceac78> chain <type_decl 
> 0x7ffff6c54a18 D.1410>>
>         sizes-gimplified unsigned SI
>         size <integer_cst 0x7ffff6c435e8 constant 32>
>         unit size <integer_cst 0x7ffff6c43600 constant 4>
>         align 64 symtab 0 alias set -1 canonical type 0x7ffff6cead20>
>    
>     arg 0 <addr_expr 0x7ffff6d1d8a0
>         type <pointer_type 0x7ffff6d33348 type <record_type 0x7ffff6ceadc8 
> Node>
>             unsigned SI size <integer_cst 0x7ffff6c435e8 32> unit size 
> <integer_cst 0x7ffff6c43600 4>
>             align 32 symtab 0 alias set -1 canonical type 0x7ffff6d333f0>
>        
>         arg 0 <parm_decl 0x7ffff6d2d300 q type <record_type 0x7ffff6ceadc8 
> Node>
>             used BLK file pr68273.c line 67 col 10
>             size <integer_cst 0x7ffff6c43948 constant 128>
>             unit size <integer_cst 0x7ffff6c43960 constant 16>
>             align 64 context <function_decl 0x7ffff6d0e700 op> arg-type 
> <record_type 0x7ffff6ceadc8 Node>>>
>     arg 1 <integer_cst 0x7ffff6d1c4f8 type <pointer_type 0x7ffff6d333f0> 
> constant 0>>
> 
>
Eric Botcazou Feb. 8, 2016, 4:31 p.m. UTC | #11 
> No, but if there is none left why would you want to "fix" SRA?

Because I'm afraid this over-aligned type might leak into other places so we 
would probably be better off not creating it in the first place, all the more 
so that it is probably useless in most cases.

For PR tree-opt/65310, why couldn't the vectorizer use the alignment of the 
pointed-to type of the addend of the MEM_REF, like for the alias set:

  # .MEM_8 = VDEF <.MEM_1(D)>
  # lhs access alignment 128+0
  MEM[(struct LorentzVector *)res_7(D)] = SR.22_44;
  # .MEM_53 = VDEF <.MEM_8>
  # lhs access alignment 32+0
  MEM[(struct LorentzVector *)res_7(D) + 4B] = SR.23_43;
  # .MEM_54 = VDEF <.MEM_53>
  # lhs access alignment 64+0
  MEM[(struct LorentzVector *)res_7(D) + 8B] = SR.24_42;
  # .MEM_55 = VDEF <.MEM_54>
  # lhs access alignment 32+0
  MEM[(struct LorentzVector *)res_7(D) + 12B] = SR.25_41;
Richard Biener Feb. 8, 2016, 4:48 p.m. UTC | #12 
On February 8, 2016 5:31:16 PM GMT+01:00, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> No, but if there is none left why would you want to "fix" SRA?
>
>Because I'm afraid this over-aligned type might leak into other places
>so we 
>would probably be better off not creating it in the first place, all
>the more 
>so that it is probably useless in most cases.
>
>For PR tree-opt/65310, why couldn't the vectorizer use the alignment of
>the 
>pointed-to type of the addend of the MEM_REF, like for the alias set:

Because that's not valid.

Richard.

>  # .MEM_8 = VDEF <.MEM_1(D)>
>  # lhs access alignment 128+0
>  MEM[(struct LorentzVector *)res_7(D)] = SR.22_44;
>  # .MEM_53 = VDEF <.MEM_8>
>  # lhs access alignment 32+0
>  MEM[(struct LorentzVector *)res_7(D) + 4B] = SR.23_43;
>  # .MEM_54 = VDEF <.MEM_53>
>  # lhs access alignment 64+0
>  MEM[(struct LorentzVector *)res_7(D) + 8B] = SR.24_42;
>  # .MEM_55 = VDEF <.MEM_54>
>  # lhs access alignment 32+0
>  MEM[(struct LorentzVector *)res_7(D) + 12B] = SR.25_41;
diff mbox

Patch

Index: gcc/tree-ssanames.c
===================================================================
--- gcc/tree-ssanames.c	(revision 233208)
+++ gcc/tree-ssanames.c	(working copy)
@@ -286,7 +286,7 @@  make_ssa_name_fn (struct function *fn, t
 
   if (TYPE_P (var))
     {
-      TREE_TYPE (t) = var;
+      TREE_TYPE (t) = TYPE_MAIN_VARIANT (var);
       SET_SSA_NAME_VAR_OR_IDENTIFIER (t, NULL_TREE);
     }
   else
Index: gcc/testsuite/gcc.target/mips/pr68273.c
===================================================================
--- gcc/testsuite/gcc.target/mips/pr68273.c	(revision 0)
+++ gcc/testsuite/gcc.target/mips/pr68273.c	(revision 0)
@@ -0,0 +1,79 @@ 
+/* { dg-do compile } */
+/* { dg-options "-w -fdump-rtl-expand" } */
+/* { dg-skip-if "" { *-*-* }  { } { "-O2" } } */
+
+extern char errbuf[];
+typedef struct Symbol
+{
+  char *name;
+}
+Symbol;
+typedef struct Tnode
+{
+}
+Tnode;
+typedef union Value
+{
+  long long i;
+}
+Value;
+typedef struct Entry
+{
+}
+Entry;
+typedef struct Table
+{
+}
+Table;
+typedef struct Node
+{
+  Tnode *typ;
+  int sto;
+  Value val;
+}
+Node;
+struct Scope
+{
+  Table *table;
+} *sp;
+static Node op (Node);
+Entry *p, *q;
+union YYSTYPE
+{
+  Symbol *sym;
+  Node rec;
+};
+typedef union YYSTYPE YYSTYPE;
+typedef short int yytype_int16;
+
+int
+yyparse (void)
+{
+  YYSTYPE yyval;
+  int yyn;
+  YYSTYPE *yyvsp;
+  while (1)
+    {
+      if (yyn == 34)
+	{
+	  if ((yyvsp[-1].rec).sto & 0x10)
+	    sprintf (errbuf, "invalid typedef qualifier for '%s'",
+		     (yyvsp[0].sym)->name);
+	  p = enter (sp->table, (yyvsp[0].sym));
+	}
+      else
+	op ((yyvsp[0].rec));
+      *++yyvsp = yyval;
+    }
+}
+
+static Node
+op (Node q)
+{
+  integer (q.typ);
+  mgtype (q.typ);
+}
+
+
+/* { dg-final { scan-rtl-dump-times "\\\(set \\\(reg:SI 5 \\\$5\\\)" 2 "expand" } }  */
+/* { dg-final { scan-rtl-dump-times "\\\(set \\\(reg:SI 6 \\\$6\\\)" 1 "expand" } }  */