Simple enhancement to -dA dump

Simple enhancement to -dA dump

[Xinliang David Li]

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

For an assembly dump, it is very useful to examine the control flow
graph with frequency and profile count information. This requires
dumping the .s file with more control flow annotations. This patch
does just that. The format of the annotation is the same as gimple
basic block dump with -fdump-tree-xxx-blocks so the same
post-processing tools can be shared to generate the .dot file. To see
bb layout, the BB sequence number is also dumped.

Bootstrapped on x86-64/linux. Regression test is on going.

Ok for checkin?

Thanks,
David


2011-04-03  Xinliang David Li  <davidxl@google.com>

        * final.c (dump_basic_block_info): New function.
        (final): Dump basic block.
        (final_scan_insn): Remove old dump.

[-- Attachment #2: dA.p --]
[-- Type: application/octet-stream, Size: 3102 bytes --]

Index: gcc/final.c
===================================================================
--- gcc/final.c	(revision 171917)
+++ gcc/final.c	(working copy)
@@ -1672,6 +1672,58 @@ final_end_function (void)
     dwarf2out_end_epilogue (last_linenum, last_filename);
 }
 \f
+
+static basic_block *start_to_bb = NULL;
+static basic_block *end_to_bb = NULL;
+static int bb_map_size = 0;
+static int bb_seqn = 0;
+
+/* Dumper helper for basic block information. FILE is the assembly
+   output file, and INSN is the instruction being emitted.  */
+
+static void
+dump_basic_block_info (FILE *file, rtx insn)
+{
+  basic_block bb;
+
+  if (!flag_debug_asm)
+    return;
+
+  if (INSN_UID (insn) < bb_map_size
+      && (bb = start_to_bb[INSN_UID (insn)]) != NULL)
+    {
+      edge e;
+      edge_iterator ei;
+
+      fprintf (file, "# BLOCK %d", bb->index);
+      if (bb->frequency)
+        fprintf (file, " freq:%d", bb->frequency);
+      if (bb->count)
+        fprintf (file, " count:" HOST_WIDEST_INT_PRINT_DEC,
+                 bb->count);
+      fprintf (file, " seq:%d", bb_seqn++);
+      fprintf (file, "\n# PRED:");
+      FOR_EACH_EDGE (e, ei, bb->preds)
+        {
+          dump_edge_info (file, e, 0);
+        }
+      fprintf (file, "\n");
+    }
+  if (INSN_UID (insn) < bb_map_size
+      && (bb = end_to_bb[INSN_UID (insn)]) != NULL)
+    {
+      edge e;
+      edge_iterator ei;
+
+      fprintf (asm_out_file, "# SUCC:");
+      FOR_EACH_EDGE (e, ei, bb->succs)
+       {
+         dump_edge_info (asm_out_file, e, 1);
+       }
+      fprintf (file, "\n");
+    }
+}
+
 /* Output assembler code for some insns: all or part of a function.
    For description of args, see `final_start_function', above.  */
 
@@ -1706,6 +1758,21 @@ final (rtx first, FILE *file, int optimi
 
   CC_STATUS_INIT;
 
+  if (flag_debug_asm)
+    {
+      basic_block bb;
+
+      bb_map_size = get_max_uid () + 1;
+      start_to_bb = XCNEWVEC (basic_block, bb_map_size);
+      end_to_bb = XCNEWVEC (basic_block, bb_map_size);
+
+      FOR_EACH_BB_REVERSE (bb)
+	{
+	  start_to_bb[INSN_UID (BB_HEAD (bb))] = bb;
+	  end_to_bb[INSN_UID (BB_END (bb))] = bb;
+	}
+    }
+
   /* Output the insns.  */
   for (insn = first; insn;)
     {
@@ -1721,8 +1788,19 @@ final (rtx first, FILE *file, int optimi
 	insn_current_address = INSN_ADDRESSES (INSN_UID (insn));
 #endif /* HAVE_ATTR_length */
 
+      dump_basic_block_info (file, insn);
       insn = final_scan_insn (insn, file, optimize_p, 0, &seen);
     }
+
+  if (flag_debug_asm)
+    {
+      free (start_to_bb);
+      free (end_to_bb);
+      start_to_bb = NULL;
+      end_to_bb = NULL;
+      bb_map_size = 0;
+      bb_seqn = 0;
+    }
 }
 \f
 const char *
@@ -1858,10 +1936,6 @@ final_scan_insn (rtx insn, FILE *file, i
 	  if (targetm.asm_out.unwind_emit)
 	    targetm.asm_out.unwind_emit (asm_out_file, insn);
 
-	  if (flag_debug_asm)
-	    fprintf (asm_out_file, "\t%s basic block %d\n",
-		     ASM_COMMENT_START, NOTE_BASIC_BLOCK (insn)->index);
-
 	  if ((*seen & (SEEN_EMITTED | SEEN_BB)) == SEEN_BB)
 	    {
 	      *seen |= SEEN_EMITTED;

[-- Attachment #3: un.s --]
[-- Type: application/octet-stream, Size: 2243 bytes --]

	.file	"un.cc"
	.text
	.p2align 4,,15
	.globl	_Z32MakeSureAllEntriesAreEqualOrZeroPii
	.type	_Z32MakeSureAllEntriesAreEqualOrZeroPii, @function
_Z32MakeSureAllEntriesAreEqualOrZeroPii:
.LFB0:
	.cfi_startproc
# BLOCK 2 freq:557 seq:0
# PRED: ENTRY [100.0%]  (fallthru)
	testl	%esi, %esi
	movl	$1, %eax
# SUCC: 3 [95.5%]  (fallthru,can_fallthru) 11 [4.5%]  (can_fallthru)
	jle	.L2
# BLOCK 3 freq:532 seq:1
# PRED: 2 [95.5%]  (fallthru,can_fallthru)
	xorl	%eax, %eax
	xorl	%ecx, %ecx
# SUCC: 6 [100.0%] 
	jmp	.L4
# BLOCK 4 freq:2387 seq:2
# PRED: 7 [50.0%]  (can_fallthru)
	.p2align 4,,10
	.p2align 3
.L12:
	cmpl	%edx, %r8d
# SUCC: 10 [4.5%]  (can_fallthru,loop_exit) 5 [95.5%]  (fallthru,can_fallthru)
	jne	.L10
# BLOCK 5 freq:7055 seq:3
# PRED: 6 [50.0%]  (can_fallthru) 4 [95.5%]  (fallthru,can_fallthru)
.L3:
	addq	$1, %rax
	cmpl	%eax, %esi
# SUCC: 6 [95.5%]  (fallthru,dfs_back,can_fallthru) 9 [4.5%]  (can_fallthru,loop_exit)
	jle	.L11
# BLOCK 6 freq:9550 seq:4
# PRED: 8 [95.5%]  (dfs_back,can_fallthru) 5 [95.5%]  (fallthru,dfs_back,can_fallthru) 3 [100.0%] 
.L4:
	movl	(%rdi,%rax,4), %edx
	testl	%edx, %edx
# SUCC: 5 [50.0%]  (can_fallthru) 7 [50.0%]  (fallthru,can_fallthru)
	je	.L3
# BLOCK 7 freq:4775 seq:5
# PRED: 6 [50.0%]  (fallthru,can_fallthru)
	testb	%cl, %cl
# SUCC: 4 [50.0%]  (can_fallthru) 8 [50.0%]  (fallthru,can_fallthru)
	jne	.L12
# BLOCK 8 freq:2388 seq:6
# PRED: 7 [50.0%]  (fallthru,can_fallthru)
	addq	$1, %rax
	movl	%edx, %r8d
	movl	$1, %ecx
	cmpl	%eax, %esi
# SUCC: 6 [95.5%]  (dfs_back,can_fallthru) 9 [4.5%]  (fallthru,can_fallthru,loop_exit)
	jg	.L4
# BLOCK 9 freq:425 seq:7
# PRED: 5 [4.5%]  (can_fallthru,loop_exit) 8 [4.5%]  (fallthru,can_fallthru,loop_exit)
.L11:
	movl	$1, %eax
# SUCC: EXIT [100.0%] 
	ret
# BLOCK 10 freq:107 seq:8
# PRED: 4 [4.5%]  (can_fallthru,loop_exit)
	.p2align 4,,10
	.p2align 3
.L10:
# SUCC: 11 [100.0%]  (fallthru,can_fallthru)
	xorl	%eax, %eax
# BLOCK 11 freq:557 seq:9
# PRED: 10 [100.0%]  (fallthru,can_fallthru) 2 [4.5%]  (can_fallthru)
.L2:
# SUCC: EXIT [100.0%] 
	rep
	ret
	.cfi_endproc
.LFE0:
	.size	_Z32MakeSureAllEntriesAreEqualOrZeroPii, .-_Z32MakeSureAllEntriesAreEqualOrZeroPii
	.ident	"GCC: (GNU) 4.7.0 20110403 (experimental)"
	.section	.note.GNU-stack,"",@progbits

[-- Attachment #4: un.s.dot --]
[-- Type: application/octet-stream, Size: 1867 bytes --]

digraph  un_s  {
	node [shape=box]
	size="11,8.5"

	 2 -> 3  [label=" [95.5%] "];
	 2 -> 11  [label=" [4.5%] "];
	2[label="\[2\] seq:0  freq:557\n	testl	%esi, %esi\n	movl	$1, %eax\n	jle	.L2\n", style=filled, color=gray]
	 3 -> 6  [label=" [100.0%] "];
	3[label="\[3\] seq:1  freq:532\n	xorl	%eax, %eax\n	xorl	%ecx, %ecx\n	jmp	.L4\n", style=filled, color=gray]
	 4 -> 10  [label=" [4.5%] "];
	 4 -> 5  [label=" [95.5%] "];
	4[label="\[4\] seq:2  freq:2387\n	.p2align 4,,10\n	.p2align 3\n.L12:\n	cmpl	%edx, %r8d\n	jne	.L10\n", style=filled, color=gray]
	 5 -> 6  [label=" [95.5%] "];
	 5 -> 9  [label=" [4.5%] "];
	5[label="\[5\] seq:3  freq:7055\n.L3:\n	addq	$1, %rax\n	cmpl	%eax, %esi\n	jle	.L11\n", style=filled, color=gray]
	 6 -> 5  [label=" [50.0%] "];
	 6 -> 7  [label=" [50.0%] "];
	6[label="\[6\] seq:4  freq:9550\n.L4:\n	movl	(%rdi,%rax,4), %edx\n	testl	%edx, %edx\n	je	.L3\n", style=filled, color=gray]
	 7 -> 4  [label=" [50.0%] "];
	 7 -> 8  [label=" [50.0%] "];
	7[label="\[7\] seq:5  freq:4775\n	testb	%cl, %cl\n	jne	.L12\n", style=filled, color=gray]
	 8 -> 6  [label=" [95.5%] "];
	 8 -> 9  [label=" [4.5%] "];
	8[label="\[8\] seq:6  freq:2388\n	addq	$1, %rax\n	movl	%edx, %r8d\n	movl	$1, %ecx\n	cmpl	%eax, %esi\n	jg	.L4\n", style=filled, color=gray]
	 9 -> EXIT  [label=" [100.0%] "];
	9[label="\[9\] seq:7  freq:425\n.L11:\n	movl	$1, %eax\n	ret\n", style=filled, color=gray]
	 10 -> 11  [label=" [100.0%] "];
	10[label="\[10\] seq:8  freq:107\n	.p2align 4,,10\n	.p2align 3\n.L10:\n	xorl	%eax, %eax\n", style=filled, color=gray]
	 11 -> EXIT  [label=" [100.0%] "];
	11[label="\[11\] seq:9  freq:557\n.L2:\n	rep\n	ret\n	.cfi_endproc\n.LFE0:\n	.size	_Z32MakeSureAllEntriesAreEqualOrZeroPii, .-_Z32MakeSureAllEntriesAreEqualOrZeroPii\n	.ident	GCC: (GNU) 4.7.0 20110403 (experimental)\n	.section	.note.GNU-stack,,@progbits\n", style=filled, color=gray]
}

Re: Simple enhancement to -dA dump

[Xinliang David Li]

Ping?


On Sun, Apr 3, 2011 at 5:54 PM, Xinliang David Li <davidxl@google.com> wrote:
> For an assembly dump, it is very useful to examine the control flow
> graph with frequency and profile count information. This requires
> dumping the .s file with more control flow annotations. This patch
> does just that. The format of the annotation is the same as gimple
> basic block dump with -fdump-tree-xxx-blocks so the same
> post-processing tools can be shared to generate the .dot file. To see
> bb layout, the BB sequence number is also dumped.
>
> Bootstrapped on x86-64/linux. Regression test is on going.
>
> Ok for checkin?
>
> Thanks,
> David
>
>
> 2011-04-03  Xinliang David Li  <davidxl@google.com>
>
>        * final.c (dump_basic_block_info): New function.
>        (final): Dump basic block.
>        (final_scan_insn): Remove old dump.
>

Re: Simple enhancement to -dA dump

[Diego Novillo]

On 04/03/2011 08:54 PM, Xinliang David Li wrote:

> +
> +      fprintf (file, "# BLOCK %d", bb->index);
> +      if (bb->frequency)
> +        fprintf (file, " freq:%d", bb->frequency);
> +      if (bb->count)
> +        fprintf (file, " count:" HOST_WIDEST_INT_PRINT_DEC,
> +                 bb->count);
> +      fprintf (file, " seq:%d", bb_seqn++);

What is this sequence number useful for?


>    /* Output the insns.  */
>    for (insn = first; insn;)
>      {
> @@ -1721,8 +1788,19 @@ final (rtx first, FILE *file, int optimi
>  	insn_current_address = INSN_ADDRESSES (INSN_UID (insn));
>  #endif /* HAVE_ATTR_length */
>
> +      dump_basic_block_info (file, insn);

Pass start_to_bb, end_to_bb and bb_seqn++ as arguments.  No need to have 
globals.  You may want to convert start_to_bb and end_to_bb to VEC(), 
but in the way you're using them, it may not make much difference in 
clarity.


> -	  if (flag_debug_asm)
> -	    fprintf (asm_out_file, "\t%s basic block %d\n",
> -		     ASM_COMMENT_START, NOTE_BASIC_BLOCK (insn)->index);
> -

There's a good number of test cases that use -dA.  I suppose that 
removing this and adding the new output did not cause new regresions?


OK with those changes.


Diego.

Re: Simple enhancement to -dA dump

[Xinliang David Li]

On Fri, Apr 8, 2011 at 5:04 AM, Diego Novillo <dnovillo@google.com> wrote:
> On 04/03/2011 08:54 PM, Xinliang David Li wrote:
>
>> +
>> +      fprintf (file, "# BLOCK %d", bb->index);
>> +      if (bb->frequency)
>> +        fprintf (file, " freq:%d", bb->frequency);
>> +      if (bb->count)
>> +        fprintf (file, " count:" HOST_WIDEST_INT_PRINT_DEC,
>> +                 bb->count);
>> +      fprintf (file, " seq:%d", bb_seqn++);
>
> What is this sequence number useful for?

Keep track of the physical order of BBs laid out in the object file --
good for diagnosing bb reorder problems.

>
>
>>   /* Output the insns.  */
>>   for (insn = first; insn;)
>>     {
>> @@ -1721,8 +1788,19 @@ final (rtx first, FILE *file, int optimi
>>        insn_current_address = INSN_ADDRESSES (INSN_UID (insn));
>>  #endif /* HAVE_ATTR_length */
>>
>> +      dump_basic_block_info (file, insn);
>
> Pass start_to_bb, end_to_bb and bb_seqn++ as arguments.  No need to have
> globals.  You may want to convert start_to_bb and end_to_bb to VEC(), but in
> the way you're using them, it may not make much difference in clarity.

Those are used to keep global states, not for the purpose parameter passing.

>
>
>> -         if (flag_debug_asm)
>> -           fprintf (asm_out_file, "\t%s basic block %d\n",
>> -                    ASM_COMMENT_START, NOTE_BASIC_BLOCK (insn)->index);
>> -
>
> There's a good number of test cases that use -dA.  I suppose that removing
> this and adding the new output did not cause new regresions?
>

No. The change has been in google's internal compiler for a long time.

Thanks,

David


>
> OK with those changes.
>
>
> Diego.
>

Re: Simple enhancement to -dA dump

[Diego Novillo]

On Fri, Apr 8, 2011 at 12:20, Xinliang David Li <davidxl@google.com> wrote:

>>>   /* Output the insns.  */
>>>   for (insn = first; insn;)
>>>     {
>>> @@ -1721,8 +1788,19 @@ final (rtx first, FILE *file, int optimi
>>>        insn_current_address = INSN_ADDRESSES (INSN_UID (insn));
>>>  #endif /* HAVE_ATTR_length */
>>>
>>> +      dump_basic_block_info (file, insn);
>>
>> Pass start_to_bb, end_to_bb and bb_seqn++ as arguments.  No need to have
>> globals.  You may want to convert start_to_bb and end_to_bb to VEC(), but in
>> the way you're using them, it may not make much difference in clarity.
>
> Those are used to keep global states, not for the purpose parameter passing.

Hm?  What are you referring to here?  Why can't start_to_bb, end_to_bb
and bb_seqn be arguments to dump_basic_block_info?


Diego.

Re: Simple enhancement to -dA dump

[Hans-Peter Nilsson]

On Sun, 3 Apr 2011, Xinliang David Li wrote:
> 2011-04-03  Xinliang David Li  <davidxl@google.com>
>
>         * final.c (dump_basic_block_info): New function.

+      fprintf (file, "# BLOCK %d", bb->index);

Random spotting: please use ASM_COMMENT_START instead of the
naked "#".

brgds, H-P