Change binary op tree display by mikedn · Pull Request #20045 · dotnet/coreclr

mikedn · 2018-09-19T16:11:21Z

This might be debatable but I find this

[000009] ------------              *  STMT      void  (IL 0x000...0x00A)
[000008] --CXG+------              \--*  RETURN    int   
[000007] a-CXG+------                 \--*  IND       ubyte 
[000014] --CXG+--R---                    \--*  INDEX_ADDR byref 
[000004] --CXG+------                       +--*  CALL      ref    Program.CheckLength
[000003] L----+------ arg1 in rdx           |  +--*  ADDR      long  
[000002] ----G+-N----                       |  |  \--*  LCL_VAR   int   (AX) V01 loc0         
[000001] -----+------ this in rcx           |  \--*  LCL_VAR   ref    V00 this         
[000006] ----G+------                       \--*  LCL_VAR   int   (AX) V01 loc0

more readable than this

[000009] ------------              *  STMT      void  (IL 0x000...0x00A)
[000008] --CXG+------              \--*  RETURN    int   
[000007] a-CXG+------                 \--*  IND       ubyte 
[000006] ----G+------                    |  /--*  LCL_VAR   int   (AX) V01 loc0         
[000014] --CXG+--R---                    \--*  INDEX_ADDR byref 
[000004] --CXG+------                       \--*  CALL      ref    Program.CheckLength
[000003] L----+------ arg1 in rdx              +--*  ADDR      long  
[000002] ----G+-N----                          |  \--*  LCL_VAR   int   (AX) V01 loc0         
[000001] -----+------ this in rcx              \--*  LCL_VAR   ref    V00 this

Unary ops display their sole operand below them, n-ary ops display all their operands below them as well, binary ops display op2 above and op1 below. Just to make ordering more messier than it already is.

mikedn · 2018-09-19T16:15:35Z

Some other random examples using the new format:

     ( 27, 11) [000171] ------------              *  STMT      void  (IL 0x0AB...0x0BC)
N010 ( 27, 11) [000170] -A------R---              \--*  ASG       int    $252
N009 (  1,  1) [000169] D------N----                 +--*  LCL_VAR   int    V02 arg2         d:9 $252
N008 ( 27, 11) [000167] ------------                 \--*  AND       int    $252
N006 ( 25,  9) [000363] ------------                    +--*  CAST      int <- long $251
N005 ( 24,  7) [000161] ------------                    |  \--*  DIV       long   $348
N003 (  3,  3) [000158] ------------                    |     +--*  SUB       long   $347
N001 (  1,  1) [000156] ------------                    |     |  +--*  LCL_VAR   long   V06 loc3         u:1 $343
N002 (  1,  1) [000157] ------------                    |     |  \--*  LCL_VAR   long   V05 loc2         u:8 $3c2
N004 (  1,  1) [000160] ------------                    |     \--*  CNS_INT   long   2 $382
N007 (  1,  1) [000364] ------------                    \--*  CNS_INT   int    -16 $51

     (  3,  3)              [000392] ------------              *  STMT      void  (IL   ???...  ???)
N009 (  3,  3)              [000391] -A------R---              \--*  ASG       int   
N008 (  1,  1)              [000389] D------N----                 +--*  LCL_VAR   int    V02 arg2         d:5
N007 (  3,  3)              [000390] ------------                 \--*  PHI       int   
N001 (  0,  0)              [000487] ------------                    +--*  PHI_ARG   int    V02 arg2         u:12
N002 (  0,  0)              [000451] ------------                    +--*  PHI_ARG   int    V02 arg2         u:4 $25e
N003 (  0,  0)              [000439] ------------                    \--*  PHI_ARG   int    V02 arg2         u:1 $100

     ( 18, 21) [000011] ------------              *  STMT      void  (IL   ???...  ???)
N017 ( 18, 21) [000010] -A-XGO------              \--*  RETURN    int    $380
N016 ( 17, 20) [000033] -A-XGO------                 \--*  COMMA     ubyte  <l:$2c1, c:$340>
N006 (  4,  4) [000022] -A-XG---R---                    +--*  ASG       ref    <l:$181, c:$1c0>
N005 (  1,  1) [000021] D------N----                    |  +--*  LCL_VAR   ref    V03 tmp1         d:2 <l:$181, c:$1c0>
N004 (  4,  4) [000019] ---XG-------                    |  \--*  IND       ref    <l:$181, c:$1c0>
N003 (  2,  2) [000035] -------N----                    |     \--*  ADD       byref  $100
N001 (  1,  1) [000001] ------------                    |        +--*  LCL_VAR   ref    V00 this         u:1 (last use) $80
N002 (  1,  1) [000034] ------------                    |        \--*  CNS_INT   long   8 field offset Fseq[currentBuffer] $c0
N015 ( 13, 16) [000032] ---XGO------                    \--*  COMMA     ubyte  <l:$2c1, c:$340>
N010 (  8, 11) [000027] ---X--------                       +--*  ARR_BOUNDS_CHECK_Rng void   <l:$187, c:$186>
N007 (  1,  1) [000008] ------------                       |  +--*  CNS_INT   int    1 $42
N009 (  3,  3) [000026] ---X--------                       |  \--*  ARR_LENGTH int    <l:$201, c:$200>
N008 (  1,  1) [000023] ------------                       |     \--*  LCL_VAR   ref    V03 tmp1         u:2 <l:$181, c:$1c0>
N014 (  5,  5) [000009] a---GO------                       \--*  IND       ubyte  <l:$2c0, c:$300>
N013 (  2,  2) [000031] -------N----                          \--*  ADD       byref  $240
N011 (  1,  1) [000024] ------------                             +--*  LCL_VAR   ref    V03 tmp1         u:2 (last use) <l:$181, c:$1c0>
N012 (  1,  1) [000030] ------------                             \--*  CNS_INT   long   17 Fseq[#ConstantIndex, #FirstElem] $c1

4creators · 2018-09-19T17:11:00Z

IMHO this arrangement of operands is easier to read. If something could be done with connecting lines (some ascii or utf-8 characters?) it would be great.

mikedn · 2018-09-19T17:22:32Z

If something could be done with connecting lines (some ascii or utf-8 characters?) it would be great.

Set environment variable COMPLUS_JitDumpASCII to 0. On Windows you'll also have to change the console to UTF-8 (using chcp 65001). You'll get this:

     ( 18, 21) [000011] ------------              ▌  STMT      void  (IL   ???...  ???)
N017 ( 18, 21) [000010] -A-XG-------              └──▌  RETURN    int
N016 ( 17, 20) [000033] -A-XG-------                 └──▌  COMMA     ubyte
N006 (  4,  4) [000022] -A-XG---R---                    ├──▌  ASG       ref
N005 (  1,  1) [000021] D------N----                    │  ├──▌  LCL_VAR   ref    V03 tmp1         d:2
N004 (  4,  4) [000019] ---XG-------                    │  └──▌  IND       ref
N003 (  2,  2) [000035] -------N----                    │     └──▌  ADD       byref
N001 (  1,  1) [000001] ------------                    │        ├──▌  LCL_VAR   ref    V00 this         u:1 (last use)
N002 (  1,  1) [000034] ------------                    │        └──▌  CNS_INT   long   8 field offset Fseq[currentBuffer]
N015 ( 13, 16) [000032] ---XG-------                    └──▌  COMMA     ubyte
N010 (  8, 11) [000027] ---X--------                       ├──▌  ARR_BOUNDS_CHECK_Rng void
N007 (  1,  1) [000008] ------------                       │  ├──▌  CNS_INT   int    1
N009 (  3,  3) [000026] ---X--------                       │  └──▌  ARR_LENGTH int
N008 (  1,  1) [000023] ------------                       │     └──▌  LCL_VAR   ref    V03 tmp1         u:2
N014 (  5,  5) [000009] a--XG-------                       └──▌  IND       ubyte
N013 (  2,  2) [000031] -------N----                          └──▌  ADD       byref
N011 (  1,  1) [000024] ------------                             ├──▌  LCL_VAR   ref    V03 tmp1         u:2 (last use)
N012 (  1,  1) [000030] ------------                             └──▌  CNS_INT   long   17 Fseq[#ConstantIndex, #FirstElem]

4creators · 2018-09-19T17:43:31Z

FWIW I remember that you can set on windows stdout stream format to utf-8 programmatically:

_setmode(_fileno(stdout), _O_U8TEXT);

IMHO we should default to utf-8 output and use COMPlus_JitDumpASCII=1 to switch to ASCII explicitly.

briansull

Looks Good

mikedn · 2018-09-19T18:18:00Z

_setmode(_fileno(stdout), _O_U8TEXT);

If you do that then you cannot use printf anymore, you have to use wprintf. Granted, that may be not much of a problem as the JIT doesn't actually use printf anyway. Its own logf could probably convert from UTF-8 to UTF-16 and call fputws instead of fputs. Might worth a try to see if it actually works. But that's something for another day.

mikedn · 2018-10-20T17:19:06Z

@dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test
test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test
test Ubuntu x64 Checked CoreFX Tests

mikedn · 2018-10-24T19:13:25Z

@dotnet-bot test Ubuntu x64 Checked CoreFX Tests
test Ubuntu16.04 arm64 Cross Checked Innerloop Build and Test

RussKeldorph · 2019-05-28T13:39:57Z

@dotnet/jit-contrib Do we want to make this change? Maybe vote with emojis at the top?

CarolEidt · 2019-05-28T14:33:17Z

I also find this more readable, and it makes the front-end dumps more consistent with the LIR dumps.

mikedn · 2019-05-29T15:04:34Z

Rebased to fix conflicts in case you decide to merge this.

CarolEidt · 2019-05-29T17:37:19Z

Given two approving reviews, and no objections, I'm going to merge this.

sandreenko · 2019-06-07T22:16:13Z

That change needs to be reflected in https://github.com/dotnet/coreclr/blob/master/Documentation/botr/ryujit-overview.md#reading-expression-trees, before we were used to lean my head to the left and read left node as the left node (and the first in execution order except for ASG) and right as the right.

Now it is not clear how to read them.. the first child is the left one, the second child is the right one.

tannergooding · 2019-06-07T22:23:18Z

before we were used to lean my head to the left and read left node as the left node (and the first in execution order except for ASG) and right as the right

If you lean your head to the right instead (opposite direction as before), then it matches again (first child would be the left node and second child is the right one).

CarolEidt · 2019-06-07T22:23:30Z

@sandreenko - good point about updating the documentation.
Personally, I never got used to the tree view, even with the leaning to the left ;-)
I have always thought about these simply as operands, so it seems logical that they are printed in order.
And given that this is the HIR "tree" view, it made sense to have them follow, though having a past that always dealt with a tuple representation, even that was a bit of a struggle for me.

CarolEidt · 2019-06-07T22:24:42Z

If you lean your head to the right instead (opposite direction as before), then it matches again (first child would be the left node and second child is the right one).

But then the parent is "below" the children - that's just a bit too weird ;-)

mikedn · 2019-06-08T08:02:05Z

That change needs to be reflected in

Yikes, I didn't realize there is documentation explaining how to read trees 😄

Now it is not clear how to read them.. the first child is the left one, the second child is the right one.

There's no left or right, there's only gtOp1 and gtOp2. Granted, in the case of GT_ASG I suppose it's weird no matter how you look at it. Hopefully we'll just get rid of it.

See #25040 for doc update.

mikedn · 2019-06-08T08:09:00Z

And FWIW I'm tempted to try displaying the tree upside down so it more closely matches the actual execution order. But that will have to wait for GT_LIST removal, then I can tree using a GenTreeVisitor instead of having to deal with the hairy display code we have now.

Commit migrated from dotnet/coreclr@2eeeac5

briansull approved these changes Sep 19, 2018

View reviewed changes

jkotas added the area-CodeGen label Dec 22, 2018

mikedn mentioned this pull request Jan 4, 2019

Stop using LIST nodes for PHI operand lists #20266

Merged

Change binary op tree display

e6c68be

mikedn force-pushed the disp-binop branch from 4038371 to e6c68be Compare May 29, 2019 15:03

CarolEidt approved these changes May 29, 2019

View reviewed changes

CarolEidt merged commit 2eeeac5 into dotnet:master May 29, 2019

mikedn deleted the disp-binop branch September 28, 2019 19:10

mikedn mentioned this pull request Jan 31, 2020

tracing/eventsource/eventsourcetrace/eventsourcetrace/eventsourcetrace.sh failure dotnet/runtime#11275

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Change binary op tree display (dotnet/coreclr#20045)

a45d8ae

Commit migrated from dotnet/coreclr@2eeeac5

Conversation

mikedn commented Sep 19, 2018

Uh oh!

mikedn commented Sep 19, 2018

Uh oh!

4creators commented Sep 19, 2018

Uh oh!

mikedn commented Sep 19, 2018

Uh oh!

4creators commented Sep 19, 2018

Uh oh!

briansull left a comment

Choose a reason for hiding this comment

Uh oh!

mikedn commented Sep 19, 2018

Uh oh!

mikedn commented Oct 20, 2018

Uh oh!

mikedn commented Oct 24, 2018

Uh oh!

RussKeldorph commented May 28, 2019

Uh oh!

CarolEidt commented May 28, 2019

Uh oh!

mikedn commented May 29, 2019

Uh oh!

CarolEidt commented May 29, 2019

Uh oh!

sandreenko commented Jun 7, 2019

Uh oh!

tannergooding commented Jun 7, 2019

Uh oh!

CarolEidt commented Jun 7, 2019

Uh oh!

CarolEidt commented Jun 7, 2019

Uh oh!

mikedn commented Jun 8, 2019

Uh oh!

mikedn commented Jun 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants