Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print indices of assertions instead of raw bitsets #54928

Merged

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Jun 29, 2021

This is to improve the experience of reading assertion propagation dumps.

Before:

GenTreeNode creates assertion:
N004 (  5,  5) [000017] ------------              *  JTRUE     void  
In BB04 New Global Constant Assertion: (337, 64) ($151,$40) Oper_Bnd { {ADD($46, $1c1)} LT  {ARR_LENGTH($80)}ADD {IntCns -1}} is  {IntCns 0} index=#08, mask=0000000000000080
BB01 valueGen = 0000000000000005 => BB03 valueGen = 0000000000000003,
BB02 valueGen = 0000000000000000
BB03 valueGen = 0000000000000010 => BB05 valueGen = 0000000000000008,
BB06 valueGen = 0000000000000000
BB04 valueGen = 00000000000000A0 => BB04 valueGen = 0000000000000060,
BB05 valueGen = 0000000000000000
AssertionPropCallback::StartMerge: BB01 in -> 0000000000000000
AssertionPropCallback::EndMerge  : BB01 in -> 0000000000000000

After:

GenTreeNode creates assertion:
N004 (  5,  5) [000017] ------------              *  JTRUE     void  
In BB04 New Global Constant Assertion: ($151,$40) Oper_Bnd { {ADD($46, $1c1)} LT  {ARR_LENGTH($80)}ADD {IntCns -1}} is  {IntCns 0}, index = #08

BB01 valueGen = #01 #03 => BB03 valueGen = #01 #02
BB02 valueGen = #NA
BB03 valueGen = #05 => BB05 valueGen = #04
BB06 valueGen = #NA
BB04 valueGen = #06 #08 => BB04 valueGen = #06 #07
BB05 valueGen = #NA

AssertionPropFlowCallback:

StartMerge: BB01 in -> #NA
EndMerge  : BB01 in -> #NA

Before:

AssertionPropCallback::StartMerge: BB02 in -> 00000000000000FF
AssertionPropCallback::Merge     : BB02 in -> 00000000000000FF, predBlock BB01 out -> 0000000000000005
AssertionPropCallback::EndMerge  : BB02 in -> 0000000000000005

AssertionPropCallback::Changed   : BB02 before out -> 00000000000000FF; after out -> 0000000000000005;
		jumpDest before out -> 00000000000000FF; jumpDest after out -> 0000000000000005;

After

StartMerge: BB02 in -> #01 #02 #03 #04 #05 #06 #07 #08
Merge     : BB02 in -> #01 #02 #03 #04 #05 #06 #07 #08; pred BB01 out -> #01 #03
EndMerge  : BB02 in -> #01 #03

Changed   : BB02 before out -> #01 #02 #03 #04 #05 #06 #07 #08; after out -> #01 #03;
        jumpDest before out -> #01 #02 #03 #04 #05 #06 #07 #08; jumpDest after out -> #01 #03;

Before:

BB01 valueIn  = 0000000000000000 valueOut = 0000000000000005 => BB03 valueOut= 0000000000000003
BB02 valueIn  = 0000000000000005 valueOut = 0000000000000005
BB03 valueIn  = 0000000000000003 valueOut = 0000000000000013 => BB05 valueOut= 000000000000000B
BB06 valueIn  = 0000000000000013 valueOut = 0000000000000013
BB04 valueIn  = 0000000000000013 valueOut = 00000000000000B3 => BB04 valueOut= 0000000000000073
BB05 valueIn  = 0000000000000003 valueOut = 0000000000000003

After:

BB01 in = #NA; out = #01 #03 => BB03 out = #01 #02
BB02 in = #01 #03; out = #01 #03
BB03 in = #01 #02; out = #01 #02 #05 => BB05 out = #01 #02 #04
BB06 in = #01 #02 #05; out = #01 #02 #05
BB04 in = #01 #02 #05; out = #01 #02 #05 #06 #08 => BB04 out = #01 #02 #05 #06 #07
BB05 in = #01 #02; out = #01 #02

(This part I am unsure about - would it be worth it to increase the vertical size of the dump to align ins with outs?)

Before:

Propagating 0000000000000013 assertions for BB04, stmt STMT00004, tree [000020], tree -> 0
Propagating 0000000000000013 assertions for BB04, stmt STMT00004, tree [000082], tree -> 0
Propagating 0000000000000013 assertions for BB04, stmt STMT00004, tree [000037], tree -> 6
Propagating 0000000000000033 assertions for BB04, stmt STMT00004, tree [000034], tree -> 0
Propagating 0000000000000033 assertions for BB04, stmt STMT00004, tree [000035], tree -> 0

After:

Propagating #01 #02 #05 for BB04, stmt STMT00004, tree [000020], tree -> #NA
Propagating #01 #02 #05 for BB04, stmt STMT00004, tree [000082], tree -> #NA
Propagating #01 #02 #05 for BB04, stmt STMT00004, tree [000037], tree -> #06
Propagating #01 #02 #05 #06 for BB04, stmt STMT00004, tree [000034], tree -> #NA
Propagating #01 #02 #05 #06 for BB04, stmt STMT00004, tree [000035], tree -> #NA

Before:

Merging assertions from pred edges of BB04 for op [000066] $ffffffff
Merge assertions from pred BB04 JTrue edge: 0000000000000073
Constant Assertion: (337, 64) ($151,$40) Oper_Bnd { {ADD($46, $1c1)} LT  {ARR_LENGTH($80)}ADD {IntCns -1}} is not  {IntCns 0} index=#07, mask=0000000000000040
The range after edge merging:<Dependent, $c0 + -2>

After:

Merging assertions from pred edges of BB04 for op [000066] $ffffffff
Merge assertions from pred BB04 JTrue edge: #01 #02 #05 #06 #07
Constant Assertion: ($151,$40) Oper_Bnd { {ADD($46, $1c1)} LT  {ARR_LENGTH($80)}ADD {IntCns -1}} is not  {IntCns 0}, index = #07
The range after edge merging:<Dependent, $c0 + -2>

The downside to this style of printing is the horizontal size of the dump increases when working with methods that have a large number of assertions. I think it is worth it.

Naturally, no diffs for this change.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 29, 2021
@SingleAccretion
Copy link
Contributor Author

cc @AndyAyersMS

Copy link
Contributor

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I like the new dump on small methods.
cc @dotnet/jit-contrib

@BruceForstall
Copy link
Member

I like the symbolic output.

IMO, we don't need to dump anything for the actual dataflow propagation; that output is only useful if you're debugging that dataflow algorithm, which shouldn't be frequent, whereas debugging which assertions are in/out could be more generally useful. I'd #if 0 that part.

I'd put in/out output (post-dataflow) on separate lines (and aligned).

@SingleAccretion
Copy link
Contributor Author

I've pushed out two changes, as per Bruce's feedback:

1) Changed the format of the final output for the assertions:

BB01:
 in   = #NA
 out  = #NA
BB02:
 in   = #NA
 out  = #02
 BB06 = #01
BB03:
 in   = #02
 out  = #02 #03 #04 #05
BB04:
 in   = #02 #03 #04 #05
 out  = #02 #03 #04 #05
 BB06 = #01 #02 #03 #04 #05
BB05:
 in   = #02 #03 #04 #05
 out  = #02 #03 #04 #05
BB06:
 in   = #NA
 out  = #02
 BB08 = #01
BB07:
 in   = #02
 out  = #02 #06
BB08:
 in   = #NA
 out  = #NA
BB09:
 in   = #02 #03 #04 #05
 out  = #02 #03 #04 #05
BB10:
 in   = #02 #03 #04 #05
 out  = #02 #03 #04 #05 #07
BB11:
 in   = #02 #03 #04 #05 #07
 out  = #02 #03 #04 #05 #07

It is not what I would call "ideal", but it is aligned.

2) Removed the printing of dataflow from the regular dumps. This is intentionally conditioned on an instance method so that it can be easily enabled via the usual environment variable config switch mechanism if needed.

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@BruceForstall
Copy link
Member

Can you rebase and ensure there are no formatting issues? (rerun jit-format)

For use in contexts where some printing method
should only be executed when "verbose" is true.
To aid in understanding what assertions are being
propagated and merged when reading the dumps.
It can still be enabled if needed.
@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Jul 8, 2021

CI failures do not appear related:

1) Libraries Test Run checked coreclr OSX x64 Debug - #55313.
2) CoreCLR Pri0 Runtime Tests Run OSX arm64 checked - completely timed out, no tests were run.

@BruceForstall BruceForstall merged commit f3b7775 into dotnet:main Jul 8, 2021
@SingleAccretion SingleAccretion deleted the Improve-Assertion-Prop-Dump branch July 8, 2021 21:22
@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants