Interleave GC Info with assembly by CarolEidt · Pull Request #42552 · dotnet/runtime

CarolEidt · 2020-09-21T19:22:40Z

Output GC info delats after the instruction (or label) that changes it.
Add COMPlus_JitDisasmWithGC option to turn on the interleaving with assembly.

Fix #41647

Output GC info delats *after* the instruction (or label) that changes it. Add `COMPlus_JitDisasmWithGC` option to turn on the interleaving with assembly. Fix dotnet#41647

CarolEidt · 2020-09-21T23:57:09Z

@dotnet/jit-contrib PTAL

CarolEidt · 2020-09-22T00:09:14Z

Output is basically what I described here. The info is printed after the impacting instruction, and multiple changes (variables or registers) are combined in a single line of output.
I've added the arg regPtrDsc entries and this snippet includes a variable going live:

G_M17768_IG14:        ; gcrefRegs=00000009 {rax rbx}, byrefRegs=00000000 {}, byref
       ; gcrRegs +[rax]
       mov      gword ptr [rsp+28H], rax
       ; GC ptr vars +{V27}
       lea      rcx, bword ptr [rax+8]
       ; byrRegs +[rcx]
       call     TreeNode:itemCheck():int:this
       ; gcrRegs -[rax]
       ; byrRegs -[rcx]
       ; gcr arg pop 0

CarolEidt · 2020-09-23T00:09:35Z

ping @dotnet/jit-contrib

sandreenko

LGTM, a few small questions/comments

sandreenko · 2020-09-23T08:23:15Z

src/coreclr/src/jit/emit.cpp

                {
                    GCtype gcType = (val & byref_OFFSET_FLAG) ? GCT_BYREF : GCT_GCREF;
-                    emitGCvarLiveUpd(offs, INT_MAX, gcType, addr);
+                    emitGCvarLiveUpd(offs, INT_MAX, gcType, addr DEBUG_ARG(num));


Thoughts: Was adding INT_MAX as a special value (that means it is a tracked variable, do not check) a TP optimization? Maybe we can revert it and always pass varNum or add an additional argument isTracked (default == false)?

This is pre-existing. I spent just a little time trying to understand why it's doing this, but the logic is not entirely clear. It doesn't seem to be a TP optimization, but I hesitate to change this as part of what's intended to be a purely dump-related PR.

sandreenko · 2020-09-23T08:36:06Z

src/coreclr/src/jit/emit.cpp

            {
                emitGCvarLiveSet(offs, gcType, addr, disp);
+#ifdef DEBUG
+                if ((EMIT_GC_VERBOSE || emitComp->opts.disasmWithGC) && ((unsigned)actualVarNum < emitComp->lvaCount) &&


(unsigned)actualVarNum is already unsigned, is not it?

Ah yes, thanks. I had originally attempted to use varNum (which is declared as int) before realizing that INT_MAX is being passed in some cases where it's an actual lclVar.

sandreenko · 2020-09-23T09:26:33Z

src/coreclr/src/jit/emit.cpp

+}
+
+//------------------------------------------------------------------------
+// emitDispGCVarDelta: Print a delta for GC variables


Nit: a wrong header?

Thanks, this was for an earlier version, will fix.

sandreenko · 2020-09-23T09:34:00Z

src/coreclr/src/jit/emit.cpp

-#else
-        bool isPinned = (desc->vpdVarNum & pinned_OFFSET_FLAG) != 0;
-
-        printf("[%08X] %s%s var died at [%s", dspPtr(desc), GCtypeStr(gcType), isPinned ? "pinned" : "",


Question: do you think that "pinned" information was not useful in the past?

I'm not sure as to how useful it was, but since that information is printed in the table of locals, it didn't seem necessary to repeat it in the dumps.

BruceForstall

LGTM

BruceForstall · 2020-09-23T17:55:50Z

src/coreclr/src/jit/emit.cpp

+// Interleaved GC info dumping.
+// We'll attempt to line this up with the opcode, which indented differently for
+// diffable and non-diffable dumps.
+// This is approximate, and is better tuned for disassembly than for jitudmps.


nit: typo: jitudmps

Thanks - will fix.

BruceForstall · 2020-09-23T17:59:46Z

src/coreclr/src/jit/emit.cpp

+
+    // We dump less information when we're only interleaving GC info with a disassembly listing,
+    // than we do in the jitdump case. (Note that the verbose argument to this method is
+    // distinct from the verbose on Compiler.


nit: missing closing paren

BruceForstall · 2020-09-23T18:04:47Z

There needs to be a follow-up to change jitutils and superpmi.py to use this new flag when generating textual asm for diffs.

AndyAyersMS · 2020-09-23T18:15:11Z

Does this print anything for untracked lifetimes?

Also perhaps (as another follow-on) we need some sort of "overall summary" that jit-analyze can use to more reliably point out gc info diffs.

CarolEidt · 2020-09-23T19:00:11Z

Does this print anything for untracked lifetimes?

No, it doesn't. I didn't really consider that as part of "interleaving" GC info, since it is independent of intra-method code locations. The information is derivable from the assembly dump because untracked lclVars have no tracked index.

Also perhaps (as another follow-on) we need some sort of "overall summary" that jit-analyze can use to more reliably point out gc info diffs.

I'm not sure what that would look like that wouldn't just be an extraction of the dumps that are interleaved. It would seem to me that anything else would of necessity include offsets. Did you have some thoughts on this?

sandreenko · 2020-09-23T19:17:55Z

src/coreclr/src/jit/emit.cpp

+                    emitComp->lvaGetDesc(actualVarNum)->lvTracked)
                {
                    VarSetOps::AddElemD(emitComp, debugThisGCrefVars,
                                        emitComp->lvaGetDesc((unsigned)actualVarNum)->lvVarIndex);


Suggested change

emitComp->lvaGetDesc((unsigned)actualVarNum)->lvVarIndex);

emitComp->lvaGetDesc(actualVarNum)->lvVarIndex);

BruceForstall · 2020-09-24T00:39:47Z

The information is derivable from the assembly dump because untracked lclVars have no tracked index.

So the key point there is that if someone makes a change that affects untracked lclVar GC info (e.g., by changing untracked to tracked, or tracked to untracked?) we'll see a textual diff?

Continuing in the line of thinking that "any change to GC info show appear as a change to asm diffs", it's possible that some things in the GC header such as PSPSym could change without corresponding change in the displayed asm. Maybe we don't worry about these rare cases for asm diffs?

CarolEidt · 2020-09-24T15:19:29Z

So the key point there is that if someone makes a change that affects untracked lclVar GC info (e.g., by changing untracked to tracked, or tracked to untracked?) we'll see a textual diff?

Right, that was my thinking in saying that.

Continuing in the line of thinking that "any change to GC info show appear as a change to asm diffs", it's possible that some things in the GC header such as PSPSym could change without corresponding change in the displayed asm. Maybe we don't worry about these rare cases for asm diffs?

I'm not sure I'd say that. It would probably be worth some investment to ensure that such diffs would be visible. I'll make a note on the original issue (which I've re-opened, as I had a fix note in this PR).

AndyAyersMS · 2020-09-24T16:17:48Z

Did you have some thoughts on this?

jit-analyze works best if it has some kind of per-method numeric value (or values) to work with, that can be easily extracted from the jit disassembly output. Ideally the value is semantically meaningful (say a size measurement) and set up so that any significant change in the thing being summarized results in a change in the value. But for GC info this may be tricky to accomplish.

Some thoughts:

print summary of gc info (# untracked lifetimes, # tracked lifetime liveness changes, gc info size) -- may miss detecting changes sometimes. This is more or less what I did for the debug info summary
print hash of gc info (either hash of the final encoded gc info blob, or hash of the interspersed text, etc) -- not semantically meaningful but would flag diffs effectively.

Interleave GC Info with assembly

dca5dcb

Output GC info delats *after* the instruction (or label) that changes it. Add `COMPlus_JitDisasmWithGC` option to turn on the interleaving with assembly. Fix dotnet#41647

Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 21, 2020

Fix release build

255c61c

sandreenko approved these changes Sep 23, 2020

View reviewed changes

BruceForstall approved these changes Sep 23, 2020

View reviewed changes

PR Feedback

6d87a05

sandreenko reviewed Sep 23, 2020

View reviewed changes

CarolEidt added 2 commits September 23, 2020 13:06

Remove 2 more unnecessary casts.

40e58d4

Formatting

a939d71

CarolEidt merged commit 5c8e3f8 into dotnet:master Sep 24, 2020

CarolEidt deleted the InterleaveGCInfo branch September 24, 2020 15:16

CarolEidt mentioned this pull request Sep 24, 2020

JIT: add option to print GC info interleaved with asm output #41647

Open

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

	emitComp->lvaGetDesc((unsigned)actualVarNum)->lvVarIndex);
	emitComp->lvaGetDesc(actualVarNum)->lvVarIndex);

Conversation

CarolEidt commented Sep 21, 2020

Uh oh!

CarolEidt commented Sep 21, 2020

Uh oh!

CarolEidt commented Sep 22, 2020

Uh oh!

CarolEidt commented Sep 23, 2020

Uh oh!

sandreenko 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!

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!

BruceForstall 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!

BruceForstall commented Sep 23, 2020

Uh oh!

AndyAyersMS commented Sep 23, 2020

Uh oh!

CarolEidt commented Sep 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BruceForstall commented Sep 24, 2020

Uh oh!

CarolEidt commented Sep 24, 2020

Uh oh!

AndyAyersMS commented Sep 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants