Update JIT dumps in RyuJIT overview doc by mikedn · Pull Request #25040 · dotnet/coreclr

mikedn · 2019-06-08T07:50:42Z

I've set out to update the JIT dumps but I also fixed some out of date info such as notes about long gone TreeNodeInfo, since it was near by.

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

Documentation/botr/ryujit-overview.md

+    +<TreeNodeInfo 0=1 1i 1f src=[allInt] int=[rax rcx rdx rbx rbp rsi rdi r8-r15 mm0-mm15] dst=[allInt] I>[--]
+  consume= 1 produce=0
+```
+The “@ 27” is the location number of the node. “internal” indicates a register that is internal to the node (in this case 2 internal registers are needed, one float (XMM on XARCH) and one int, as temporaries for copying). “regmask” indicates the register constraints for the `RefPosition`.


@CarolEidt I removed any description related to TreeNodeInfo even if it's still in the dump. It looks rather useless and redundant to me, perhaps we should just remove it from the dump? Everything is about refpositions and intervals now.

I'd left the TreeNodeInfo dumps in originally in order to make the diffs easier, but that was long ago, so definitely time to clean it up in teh actual dumps (though that can obviously be deferred to a future PR).

I'll take a look, probably it's easy to cleanup and then it doesn't worth a separate PR just for removing some dump code.

Hmm, I was checking to see if there's anything we would lose by simply deleting the TreeNodeInfo message and noticed something odd about the RefPosition dump. The <RefPosition... message is generated at the end of newRefPosition but in many cases the caller will modify the ref position after creating it:

RefPosition* defRefPosition = newRefPosition(interval, currentLoc + 1, RefTypeDef, tree, dstCandidates, multiRegIdx); if (tree->IsUnusedValue()) { defRefPosition->isLocalDefUse = true; defRefPosition->lastUse = true; }

It took me a while to understand why "local" wasn't appearing in the dump. Well, "local" appears but only in the subsequent ref position dumps. Not sure what would be the best way to avoid this confusing behavior. The easiest way would be for the caller to do the dump when it has finished setting up the ref position but there's a good chance that we'll forget to dump when we add new ref position creating code in the future.

I removed the entire dumpNodeInfo function. Might be debatable as some of the information it shows isn't readily available somewhere else, srcCount and dstCount in particular. But those are IMO weird, some double bookkeeping that just complicated things.

Yes, there's probably a better approach to sanity checking.

Documentation/botr/ryujit-overview.md

sandreenko · 2019-07-22T17:31:53Z

cc @dotnet/jit-contrib

CarolEidt

Thanks for the cleanup @mikedn! If you'd be willing to add a few words about the order of the ASG operators and about containedness analysis in Lowering, that would be great.

Documentation/botr/ryujit-overview.md

CarolEidt · 2019-07-22T21:35:31Z

Documentation/botr/ryujit-overview.md

+ t4 =    LCL_VAR   double V01 arg1         u:1 $81
+     ┌──▌  t3     double 
+     ├──▌  t4     double 
+ t5 = ▌  MUL       double $140


Somewhere (in one of my multiple branches that are on hold) I've fixed the fact that the operands no longer line up with the consuming op.

Not a fan of the way operands are displayed in LIR to begin with 😁. I'd rather do something more like MUL double $140 [t3, t4], that would remove a ton of noise and waste of vertical space from the LIR dumps.

I agree. Now that LIR is no longer required to maintain a specific kind of tree order (and hence the edges no longer actually connect to the child node), this doesn't really add any value.

CarolEidt · 2019-07-22T23:07:06Z

Documentation/botr/ryujit-overview.md

-    t7   = +          byref
-           ┌──▌  t7   ref
-    t8   = indir      int
+It does an execution-order traversal that performs context-dependent transformations such as expanding switch statements (using a switch table or a series of conditional branches), constructing addressing modes, determining the code generation strategy for block assignments (e.g. `GT_STORE_BLK`) which may become helper calls, unrolled loops, or an instruction like `rep stos` etc.  For example, this:


This should also mention the containedness analysis, which used to be part of the register allocator, but is now done as part of Lowering.

Expanded to mention reg optional, containment and generating target specific instructions such as BT.

CarolEidt · 2019-07-22T23:09:46Z

Documentation/botr/ryujit-overview.md

+    +<TreeNodeInfo 0=1 1i 1f src=[allInt] int=[rax rcx rdx rbx rbp rsi rdi r8-r15 mm0-mm15] dst=[allInt] I>[--]
+  consume= 1 produce=0
+```
+The “@ 27” is the location number of the node. “internal” indicates a register that is internal to the node (in this case 2 internal registers are needed, one float (XMM on XARCH) and one int, as temporaries for copying). “regmask” indicates the register constraints for the `RefPosition`.


I'd left the TreeNodeInfo dumps in originally in order to make the diffs easier, but that was long ago, so definitely time to clean it up in teh actual dumps (though that can obviously be deferred to a future PR).

sandreenko

LGTM, thank you.

CarolEidt

Thanks for this cleanup!

CarolEidt · 2019-07-24T05:11:19Z

Documentation/botr/ryujit-overview.md

+* determining the code generation strategy for block assignments (e.g. `GT_STORE_BLK`) which may become helper calls, unrolled loops, or an instruction like `rep stos`
+* generating machine specific instructions (e.g. generating the `BT` x86/64 instruction)
+* mark "contained" nodes - such a node does not generate any code and relies on its user to include the node's operation in its own codegen (e.g. memory operands, immediate operands)
+* mark "reg optional" nodes - despite the name, such a node may produce a value in a register but its user does not require a register and can consume the value directly from a memory location


I'm curious about what you mean by "despite the name" - to me, "reg optional" implies that it can have a register or not - either way.

The reg optional flag is set on the node that produces the value but ends up being used by the "use" ref position of that node. So it's really "reg optional uses", except that a use isn't a tangible entity in the IR so it's a bit confusing to call it that.

Ah, yes I see that now. I'd love to see us support reg-optional defs as well, but that wasn't quite as straightforward as I'd initial thought - shouldn't be rocket science though.

I suppose it might be possible to get rid of SetRegOptional from GenTree and instead put this logic in LSRA's BuildNode so that reg optional gets set directly on the ref position. But then the reg optional logic piggybacks on the containment logic so attempting to move it may result in some duplication.

CarolEidt · 2019-07-24T05:13:12Z

Documentation/botr/ryujit-overview.md

+    +<TreeNodeInfo 0=1 1i 1f src=[allInt] int=[rax rcx rdx rbx rbp rsi rdi r8-r15 mm0-mm15] dst=[allInt] I>[--]
+  consume= 1 produce=0
+```
+The “@ 27” is the location number of the node. “internal” indicates a register that is internal to the node (in this case 2 internal registers are needed, one float (XMM on XARCH) and one int, as temporaries for copying). “regmask” indicates the register constraints for the `RefPosition`.


Yes, there's probably a better approach to sanity checking.

* Update JIT dumps in RyuJIT overview doc * Remove the 2 pass mention from lowering * RyuJIT overview update feedback * Delete LinearScan::dumpNodeInfo * Markdown fixes Commit migrated from dotnet/coreclr@b07a8c5

mikedn mentioned this pull request Jun 8, 2019

Change binary op tree display #20045

Merged

mikedn commented Jun 8, 2019

View reviewed changes

sandreenko reviewed Jul 22, 2019

View reviewed changes

Documentation/botr/ryujit-overview.md Outdated Show resolved Hide resolved

Documentation/botr/ryujit-overview.md Show resolved Hide resolved

CarolEidt reviewed Jul 22, 2019

View reviewed changes

mikedn added 4 commits July 23, 2019 21:37

Update JIT dumps in RyuJIT overview doc

4377ba3

Remove the 2 pass mention from lowering

9659a63

RyuJIT overview update feedback

aae0fff

Delete LinearScan::dumpNodeInfo

0202e32

mikedn force-pushed the patch-1 branch from 5619828 to 0202e32 Compare July 23, 2019 19:50

Markdown fixes

56e3edb

sandreenko approved these changes Jul 24, 2019

View reviewed changes

CarolEidt approved these changes Jul 24, 2019

View reviewed changes

CarolEidt merged commit b07a8c5 into dotnet:master Jul 24, 2019

mikedn deleted the patch-1 branch September 28, 2019 19:06

Conversation

mikedn commented Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sandreenko commented Jul 22, 2019

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandreenko left a comment

Choose a reason for hiding this comment

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mikedn commented Jun 8, 2019 •

edited

Loading