[RFC] Confusing fall through BB pc in conditional jump

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[RFC] Confusing fall through BB pc in conditional jump

Kewen.Lin-2
Hi all,

    6: NOTE_INSN_BASIC_BLOCK 2
        ....
   12: r135:CC=cmp(r122:DI,0)
   13: pc={(r135:CC!=0)?L52:pc}
      REG_DEAD r135:CC
      REG_BR_PROB 1041558836
   31: L31:
   17: NOTE_INSN_BASIC_BLOCK 3

The above RTL seq is from pass doloop dumping with -fdump-rtl-all-slim, I misunderstood that:
the fall through BB of BB 2 is BB 3, since BB 3 is placed just next to BB 2.  
Then I found the contradiction that BB 3 will have some uninitialized regs if it's true.

I can get the exact information with "-blocks" dumping and even detailed one with "-details".
But I'm thinking whether it's worth to giving some information on "-slim" dump (or more
exactly without "-blocks") to avoid some confusion especially for new comers like me.
Or is it unnecessary? since we have "-blocks" options for that and want to keep it "slim"?

Any comments on that?

Thanks in advance!


-------------------------------

The below patch is to append the hint on pc target when we find jump insn with if-then-else
and the target BB with known code label.

The dumping will look like:
    6: NOTE_INSN_BASIC_BLOCK 2
        ...
   12: r135:CC=cmp(r122:DI,0)
   13: pc={(r135:CC!=0)?L52:pc}
      REG_DEAD r135:CC
      REG_BR_PROB 1041558836
;;  pc (fall through) -> L67
   31: L31:
   17: NOTE_INSN_BASIC_BLOCK 3


diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
index a1ca5992c41..608bcd130c5 100644
--- a/gcc/cfgrtl.c
+++ b/gcc/cfgrtl.c
@@ -2164,7 +2164,27 @@ rtl_dump_bb (FILE *outf, basic_block bb, int indent, dump_flags_t flags)
     }

 }
-^L
+
+/* For dumping without specifying basic blocks option, when we see pc is one of
+   jump targets, it's easy to misunderstand the next basic block is the
+   fallthrough one, but it's not so true.  This function is to dump some hints
+   for that.  */
+
+static void
+dump_hints_for_jump_target_pc (FILE *outf, basic_block bb)
+{
+  gcc_assert (outf);
+  edge e = FALLTHRU_EDGE (bb);
+  basic_block dest = e->dest;
+  rtx_insn *label = BB_HEAD (dest);
+  if (!LABEL_P (label))
+    return;
+
+  fputs (";; ", outf);
+  fprintf (outf, " pc (fall through) -> L%d", INSN_UID (label));
+  fputc ('\n', outf);
+}
+
 /* Like dump_function_to_file, but for RTL.  Print out dataflow information
    for the start of each basic block.  FLAGS are the TDF_* masks documented
    in dumpfile.h.  */
@@ -2255,6 +2275,14 @@ print_rtl_with_bb (FILE *outf, const rtx_insn *rtx_first, dump_flags_t flags)
                  putc ('\n', outf);
                }
            }
+         else if (GET_CODE (tmp_rtx) == JUMP_INSN
+                  && GET_CODE (PATTERN (tmp_rtx)) == SET)
+           {
+             bb = BLOCK_FOR_INSN (tmp_rtx);
+             const_rtx src = SET_SRC (PATTERN (tmp_rtx));
+             if (bb != NULL && GET_CODE (src) == IF_THEN_ELSE)
+               dump_hints_for_jump_target_pc (outf, bb);
+           }
        }

       free (start);

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] Confusing fall through BB pc in conditional jump

Segher Boessenkool
Hi Kewen,

On Thu, Jun 27, 2019 at 10:32:18AM +0800, Kewen.Lin wrote:

>     6: NOTE_INSN_BASIC_BLOCK 2
> ....
>    12: r135:CC=cmp(r122:DI,0)
>    13: pc={(r135:CC!=0)?L52:pc}
>       REG_DEAD r135:CC
>       REG_BR_PROB 1041558836
>    31: L31:
>    17: NOTE_INSN_BASIC_BLOCK 3
>
> The above RTL seq is from pass doloop dumping with -fdump-rtl-all-slim, I misunderstood that:
> the fall through BB of BB 2 is BB 3, since BB 3 is placed just next to BB 2.  
> Then I found the contradiction that BB 3 will have some uninitialized regs if it's true.
>
> I can get the exact information with "-blocks" dumping and even detailed one with "-details".
> But I'm thinking whether it's worth to giving some information on "-slim" dump (or more
> exactly without "-blocks") to avoid some confusion especially for new comers like me.
> Or is it unnecessary? since we have "-blocks" options for that and want to keep it "slim"?

It's probably a good idea to spend a line on this even in slim mode, it's
really easy to miss and get confused and waste time without it.  Nice :-)

> The dumping will look like:
>     6: NOTE_INSN_BASIC_BLOCK 2
> ...
>    12: r135:CC=cmp(r122:DI,0)
>    13: pc={(r135:CC!=0)?L52:pc}
>       REG_DEAD r135:CC
>       REG_BR_PROB 1041558836
> ;;  pc (fall through) -> L67
>    31: L31:
>    17: NOTE_INSN_BASIC_BLOCK 3

I think it should say the BB number it falls through to.  Is the label
number (insn number) useful?  Maybe the BB number isn't always printed
in slim dumps?

All this only happens in cfglayout mode btw, you might want to restrict
this to that: in normal rtl mode fallthrough does fall through to the
next insn always.

Maybe slim dumps should always spend a line on showing basic block
boundaries?  Something visual like maybe
; ------------------------------
or
; BB 456 ------------------------------
or
; ------------ falls through to BB 123
or
; ------------ falls through to BB 123
; BB 456 ------------------------------

I don't use slim dumps much, they lose too much information, but maybe
that can be fixed :-)


Segher
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] Confusing fall through BB pc in conditional jump

Kewen.Lin-2
Hi Segher,

Thanks a lot for the comments.

on 2019/6/28 上午9:32,  wrote:

> Hi Kewen,
>
> On Thu, Jun 27, 2019 at 10:32:18AM +0800, Kewen.Lin wrote:
>>     6: NOTE_INSN_BASIC_BLOCK 2
>> ....
>>    12: r135:CC=cmp(r122:DI,0)
>>    13: pc={(r135:CC!=0)?L52:pc}
>>       REG_DEAD r135:CC
>>       REG_BR_PROB 1041558836
>>    31: L31:
>>    17: NOTE_INSN_BASIC_BLOCK 3
>>
>> The above RTL seq is from pass doloop dumping with -fdump-rtl-all-slim, I misunderstood that:
>> the fall through BB of BB 2 is BB 3, since BB 3 is placed just next to BB 2.  
>> Then I found the contradiction that BB 3 will have some uninitialized regs if it's true.
>>
>> I can get the exact information with "-blocks" dumping and even detailed one with "-details".
>> But I'm thinking whether it's worth to giving some information on "-slim" dump (or more
>> exactly without "-blocks") to avoid some confusion especially for new comers like me.
>> Or is it unnecessary? since we have "-blocks" options for that and want to keep it "slim"?
>
> It's probably a good idea to spend a line on this even in slim mode, it's
> really easy to miss and get confused and waste time without it.  Nice :-)
>
>> The dumping will look like:
>>     6: NOTE_INSN_BASIC_BLOCK 2
>> ...
>>    12: r135:CC=cmp(r122:DI,0)
>>    13: pc={(r135:CC!=0)?L52:pc}
>>       REG_DEAD r135:CC
>>       REG_BR_PROB 1041558836
>> ;;  pc (fall through) -> L67
>>    31: L31:
>>    17: NOTE_INSN_BASIC_BLOCK 3
>
> I think it should say the BB number it falls through to.  Is the label
> number (insn number) useful?  Maybe the BB number isn't always printed
> in slim dumps?

OK, to use the label was meant to keep the same as what we print for jump
targets of jump insn.  Yes, I think it's better to use BB number.
I think we can get the BB number from NOTE_INSN_BASIC_BLOCK now, it's
always printed in slim dumps.

>
> All this only happens in cfglayout mode btw, you might want to restrict
> this to that: in normal rtl mode fallthrough does fall through to the
> next insn always.

Good idea!

>
> Maybe slim dumps should always spend a line on showing basic block
> boundaries?  Something visual like maybe
> ; ------------------------------
> or
> ; BB 456 ------------------------------
> or
> ; ------------ falls through to BB 123
> or
> ; ------------ falls through to BB 123
> ; BB 456 ------------------------------
>

It seems the current NOTE_INSN_BASIC_BLOCK is enough for the beginning?

> I don't use slim dumps much, they lose too much information, but maybe
> that can be fixed :-)

OK, since I haven't got used to lisp format, the slim seems more
understandable to me.  If it's not used too much, maybe this confusion
and possible improvement is too trivial.  :)


Thanks,
Kewen

>
>
> Segher
>

Reply | Threaded
Open this post in threaded view
|

Re: [RFC] Confusing fall through BB pc in conditional jump

Segher Boessenkool
Hi Kewen,

On Fri, Jun 28, 2019 at 09:56:58AM +0800, Kewen.Lin wrote:

> > It's probably a good idea to spend a line on this even in slim mode, it's
> > really easy to miss and get confused and waste time without it.  Nice :-)
> >
> >> The dumping will look like:
> >>     6: NOTE_INSN_BASIC_BLOCK 2
> >> ...
> >>    12: r135:CC=cmp(r122:DI,0)
> >>    13: pc={(r135:CC!=0)?L52:pc}
> >>       REG_DEAD r135:CC
> >>       REG_BR_PROB 1041558836
> >> ;;  pc (fall through) -> L67
> >>    31: L31:
> >>    17: NOTE_INSN_BASIC_BLOCK 3
> >
> > I think it should say the BB number it falls through to.  Is the label
> > number (insn number) useful?  Maybe the BB number isn't always printed
> > in slim dumps?
>
> OK, to use the label was meant to keep the same as what we print for jump
> targets of jump insn.  Yes, I think it's better to use BB number.
> I think we can get the BB number from NOTE_INSN_BASIC_BLOCK now, it's
> always printed in slim dumps.

Ah cool.  I don't often look at "normal" slim dumps, sorry if some things
I said were more confusing than helpful :-)  (I see the thing combine
prints at the start of the dump a lot, but that skips all non-insns.  I
should change that to make BB boundaries visible, it's pretty crucial for
combine :-) )

> > Maybe slim dumps should always spend a line on showing basic block
> > boundaries?  Something visual like maybe
> > ; ------------------------------
> > or
> > ; BB 456 ------------------------------
> > or
> > ; ------------ falls through to BB 123
> > or
> > ; ------------ falls through to BB 123
> > ; BB 456 ------------------------------
> >
>
> It seems the current NOTE_INSN_BASIC_BLOCK is enough for the beginning?

Yes, you are right.  That note itself is probably enough to make it obvious
there is a BB boundary, when skimming a dump.

> > I don't use slim dumps much, they lose too much information, but maybe
> > that can be fixed :-)
>
> OK, since I haven't got used to lisp format, the slim seems more
> understandable to me.  If it's not used too much, maybe this confusion
> and possible improvement is too trivial.  :)

Slim dumps are much more compact, so you can do some things with it that
you cannot with "full" dumps.  But they miss just a little bit *too* much
information; your patch helps to fix that.  I'm all for it :-)


[ Unrelated...  One of my biggest problems with slim dumps...  subregs...
  If it says

    r128:CC=cmp(r124:DI#4,0)

  then in what mode is the compare actually done?  Now because I know what
  target this is in which configuration, the #4 is a good hint it is SImode,
  but what if it was LE so it would say #0?  And for some ops you really
  cannot guess at all, say when picking apart a register. ]


Segher