Re: [PATCH v2] Dump BB number when dumping a BB with label.

[Richard Biener <richard.guenther@gmail.com>]

On Mon, Jul 31, 2017 at 8:43 AM, Martin Liška <mliska@suse.cz> wrote:
> On 07/28/2017 01:21 PM, Richard Biener wrote:
>> On Fri, Jul 28, 2017 at 12:52 PM, Martin Liška <mliska@suse.cz> wrote:
>>> On 07/28/2017 09:58 AM, Richard Biener wrote:
>>>> On Fri, Jul 28, 2017 at 9:50 AM, Martin Liška <mliska@suse.cz> wrote:
>>>>> On 07/28/2017 09:21 AM, Richard Biener wrote:
>>>>>> On Thu, Jul 27, 2017 at 4:24 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>>>> Hi.
>>>>>>>
>>>>>>> Following simple patch adds support for dumping of BBs when it's a BB
>>>>>>> that contains a label. That makes it easier for debugging as one can
>>>>>>> find destination for an edge in dump file.
>>>>>>>
>>>>>>> Sample, before:
>>>>>>>
>>>>>>> foo (int a)
>>>>>>> {
>>>>>>>   int D.1821;
>>>>>>>   int _1;
>>>>>>>   int _4;
>>>>>>>   int _5;
>>>>>>>
>>>>>>>   <bb 2> [0.00%] [count: INV]:
>>>>>>>   switch (a_2(D)) <default: <L2> [INV] [count: INV], case 0: <L0> [INV] [count: INV], case 1: <L1> [INV] [count: INV]>
>>>>>>>
>>>>>>> <L0> [0.00%] [count: INV]:
>>>>>>>   a_3 = a_2(D) + 2;
>>>>>>>
>>>>>>> <L1> [0.00%] [count: INV]:
>>>>>>>   _4 = 2;
>>>>>>>   goto <bb 6> (<L3>); [INV] [count: INV]
>>>>>>>
>>>>>>> <L2> [0.00%] [count: INV]:
>>>>>>>   _5 = 123;
>>>>>>>
>>>>>>>   # _1 = PHI <_4(4), _5(5)>
>>>>>>> <L3> [0.00%] [count: INV]:
>>>>>>>   return _1;
>>>>>>>
>>>>>>> }
>>>>>>>
>>>>>>> After:
>>>>>>>
>>>>>>> foo (int a)
>>>>>>> {
>>>>>>>   int D.1821;
>>>>>>>   int _1;
>>>>>>>   int _4;
>>>>>>>   int _5;
>>>>>>>
>>>>>>>   <bb 2> [0.00%] [count: INV]:
>>>>>>>   switch (a_2(D)) <default: <L2> [INV] [count: INV], case 0: <L0> [INV] [count: INV], case 1: <L1> [INV] [count: INV]>
>>>>>>>
>>>>>>> <L0> (<bb 3>) [0.00%] [count: INV]:
>>>>>>>   a_3 = a_2(D) + 2;
>>>>>>>
>>>>>>> <L1> (<bb 4>) [0.00%] [count: INV]:
>>>>>>>   _4 = 2;
>>>>>>>   goto <bb 6> (<L3>); [INV] [count: INV]
>>>>>>>
>>>>>>> <L2> (<bb 5>) [0.00%] [count: INV]:
>>>>>>>   _5 = 123;
>>>>>>>
>>>>>>>   # _1 = PHI <_4(4), _5(5)>
>>>>>>> <L3> (<bb 6>) [0.00%] [count: INV]:
>>>>>>>   return _1;
>>>>>>>
>>>>>>> }
>>>>>>>
>>>>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>>>>>
>>>>>>> Thoughts?
>>>>>>
>>>>>> I think I prefer to always see
>>>>>>
>>>>>>   <bb 3> ....:
>>>>>>
>>>>>> and if there's a label just dump that as well, thus
>>>>>>
>>>>>>   <bb 3> ....:
>>>>>>   L0:
>>>>>>
>>>>>> I think that's how we dump the case with multiple labels.  And always use the
>>>>>> implicit bb N when dumping destinations (in gotos, switches, etc).
>>>>>>
>>>>>> That is, what we have now is IMHO premature prettifying losing BB
>>>>>> indices in the dumps
>>>>>> unnecessarily.
>>>>>>
>>>>>> Richard.
>>>>>
>>>>> Hi.
>>>>>
>>>>> I like your ideas, there's difference in between 7.1 and modified trunk:
>>>>>
>>>>> foo (int a)
>>>>> {
>>>>>   int D.1824;
>>>>>   int _1;
>>>>>   int _4;
>>>>>   int _6;
>>>>>
>>>>>   <bb 2> [0.00%] [count: INV]:
>>>>>   switch (a_2(D)) <default: <L2> [INV] [count: INV], case 0: <L0> [INV] [count: INV], case 1: <L1> [INV] [count: INV]>
>>>>>
>>>>> <L0> [0.00%] [count: INV]:
>>>>>   a_3 = a_2(D) + 2;
>>>>>
>>>>> <L1> [0.00%] [count: INV]:
>>>>>   _4 = 2;
>>>>>   goto <bb 8> (<L6>); [INV] [count: INV]
>>>>>
>>>>> <L2> [0.00%] [count: INV]:
>>>>>
>>>>>   <bb 6> [0.00%] [count: INV]:
>>>>>   a_5 = a_2(D) + 2;
>>>>>
>>>>> label_XXX [0.00%] [count: INV]:
>>>>> label_YYY [0.00%] [count: INV]:
>>>>>   _6 = 101;
>>>>>
>>>>>   # _1 = PHI <_4(4), _6(7)>
>>>>> <L6> [0.00%] [count: INV]:
>>>>>   return _1;
>>>>>
>>>>> }
>>>>>
>>>>> after:
>>>>>
>>>>> foo (int a)
>>>>> {
>>>>>   int D.1824;
>>>>>   int _1;
>>>>>   int _4;
>>>>>   int _6;
>>>>>
>>>>>   <bb 2> [0.00%] [count: INV]:
>>>>>   switch (a_2(D)) <default: <bb 5> [INV] [count: INV], case 0: <bb 3> [INV] [count: INV], case 1: <bb 4> [INV] [count: INV]>
>>>>>
>>>>>   <bb 3> [0.00%] [count: INV]:
>>>>> <L0>:
>>>>>   a_3 = a_2(D) + 2;
>>>>>
>>>>>   <bb 4> [0.00%] [count: INV]:
>>>>> <L1>:
>>>>>   _4 = 2;
>>>>>   goto <bb 8>; [INV] [count: INV]
>>>>>
>>>>>   <bb 5> [0.00%] [count: INV]:
>>>>> <L2>:
>>>>>
>>>>>   <bb 6> [0.00%] [count: INV]:
>>>>>   a_5 = a_2(D) + 2;
>>>>>
>>>>>   <bb 7> [0.00%] [count: INV]:
>>>>> label_XXX:
>>>>> label_YYY:
>>>>>   _6 = 101;
>>>>>
>>>>>   <bb 8> [0.00%] [count: INV]:
>>>>>   # _1 = PHI <_4(4), _6(7)>
>>>>> <L6>:
>>>>>   return _1;
>>>>>
>>>>> }
>>>>>
>>>>> Do you like it? What about indentation of labels, should I increase it or leave it?
>>>>
>>>> Leave it.
>>>>
>>>>> I guess there will be some tests that will need to be adjusted.
>>>>
>>>> I guess so.
>>>>
>>>> I think <L0>: and friends are all DECL_ARTIFICIAL -- maybe we can avoid dumping
>>>> them?  Hmm, I guess doing it like above, while it preserves BB numbering, does
>>>> reflect the actual IL a bit less so I guess I'd leave the <L0>s in
>>>> switches (those
>>>> have labels) and gotos if there's still the label args (not in case of
>>>> we are just
>>>> dumping CFG edges).
>>>
>>> Good, thus said there's how it will look like:
>>>
>>> $ cat /tmp/switch.c
>>> int c;
>>>
>>> int foo(int a)
>>> {
>>>   switch (a)
>>>   {
>>>   case 0:
>>>     a += 2;
>>>   case 1:
>>>     if (c)
>>>       goto label_XXX;
>>>     return 2;
>>>   default:
>>>     break;
>>>   }
>>>
>>>   a += 2;
>>>
>>> label_XXX:
>>> label_YYY:
>>>   return 99 + 2;
>>> }
>>>
>>> $ ./xgcc -B. /tmp/switch.c -fdump-tree-optimized=/dev/stdout
>>>
>>> ;; Function foo (foo, funcdef_no=0, decl_uid=1816, cgraph_uid=0, symbol_order=1)
>>>
>>> foo (int a)
>>> {
>>>   int D.1827;
>>>   int c.0_1;
>>>   int _2;
>>>   int _6;
>>>   int _8;
>>>
>>>   <bb 2> [0.00%] [count: INV]:
>>>   switch (a_3(D)) <default: <L4> [INV] [count: INV], case 0: <L0> [INV] [count: INV], case 1: <L1> [INV] [count: INV]>
>>>
>>>   <bb 3> [0.00%] [count: INV]:
>>> <L0>:
>>>   a_4 = a_3(D) + 2;
>>>
>>>   <bb 4> [0.00%] [count: INV]:
>>> <L1>:
>>>   c.0_1 = c;
>>>   if (c.0_1 != 0)
>>>     goto <bb 5>; [INV] [count: INV]
>>>   else
>>>     goto <bb 6>; [INV] [count: INV]
>>>
>>>   <bb 5> [0.00%] [count: INV]:
>>>   goto <bb 9>; [INV] [count: INV]
>>>
>>>   <bb 6> [0.00%] [count: INV]:
>>>   _6 = 2;
>>>   goto <bb 10>; [INV] [count: INV]
>>>
>>>   <bb 7> [0.00%] [count: INV]:
>>> <L4>:
>>>
>>>   <bb 8> [0.00%] [count: INV]:
>>>   a_7 = a_3(D) + 2;
>>>
>>>   <bb 9> [0.00%] [count: INV]:
>>> label_XXX:
>>> label_YYY:
>>>   _8 = 101;
>>>
>>>   <bb 10> [0.00%] [count: INV]:
>>>   # _2 = PHI <_6(6), _8(9)>
>>> <L8>:
>>>   return _2;
>>>
>>> }
>>>
>>>
>>> Note that edge bb_5->bb_9 is represented after gimplification by implicit edge, not by goto. But:
>>>
>>> ./xgcc -B. /tmp/switch.c -fdump-tree-lower=/dev/stdout
>>>
>>> ;; Function foo (foo, funcdef_no=0, decl_uid=1816, cgraph_uid=0, symbol_order=1)
>>>
>>> foo (int a)
>>> {
>>>   int D.1827;
>>>
>>>   switch (a) <default: <D.1821>, case 0: <D.1818>, case 1: <D.1819>>
>>>   <D.1818>:
>>>   a = a + 2;
>>>   <D.1819>:
>>>   c.0_1 = c;
>>>   if (c.0_1 != 0) goto <D.1825>; else goto <D.1826>;
>>>   <D.1825>:
>>>   goto label_XXX;
>>>   <D.1826>:
>>>   D.1827 = 2;
>>>   goto <D.1828>;
>>>   <D.1821>:
>>>   goto <D.1822>;
>>>   <D.1822>:
>>>   a = a + 2;
>>>   label_XXX:
>>>   label_YYY:
>>>   D.1827 = 101;
>>>   goto <D.1828>;
>>>   <D.1828>:
>>>   return D.1827;
>>> }
>>>
>>> There labels are dumped properly. If it's ok I'll start working on test-suite transition.
>>
>> Yes.  Looks good to me now.
>>
>> That said... if the fallout is very big we might consider switching to
>> -gimple style dumping
>> unconditionally?
>>
>> Richard.
>
> Hello.
>
> Sending second version of the patch. Eventually it shows that fallout for test suite was minimal.
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>
> Ready to be installed?

Ok.  Nice that it also simplifies code.

Thanks,
Richard.

> Martin
>
>>
>>> Martin
>>>
>>>>
>>>> Richard.
>>>>
>>>>> Martin
>>>>>
>>>>>>
>>>>>>> Martin
>>>>>>>
>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>
>>>>>>> 2017-07-27  Martin Liska  <mliska@suse.cz>
>>>>>>>
>>>>>>>         * gcc.dg/builtin-unreachable-6.c: Update scanned pattern.
>>>>>>>         * gcc.dg/tree-ssa/attr-hotcold-2.c: Likewise.
>>>>>>>         * gcc.dg/tree-ssa/ssa-ccp-18.c: Likewise.
>>>>>>>
>>>>>>> gcc/ChangeLog:
>>>>>>>
>>>>>>> 2017-07-27  Martin Liska  <mliska@suse.cz>
>>>>>>>
>>>>>>>         * gimple-pretty-print.c (dump_gimple_label): Dump BB number.
>>>>>>> ---
>>>>>>>  gcc/gimple-pretty-print.c                      | 6 +++++-
>>>>>>>  gcc/testsuite/gcc.dg/builtin-unreachable-6.c   | 2 +-
>>>>>>>  gcc/testsuite/gcc.dg/tree-ssa/attr-hotcold-2.c | 4 ++--
>>>>>>>  gcc/testsuite/gcc.dg/tree-ssa/ssa-ccp-18.c     | 3 +--
>>>>>>>  4 files changed, 9 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>>
>>>>>
>>>
>