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

Change debug info default variable tracking system #2107

Merged
merged 5 commits into from
Feb 26, 2020
Merged

Change debug info default variable tracking system #2107

merged 5 commits into from
Feb 26, 2020

Conversation

BrianBohe
Copy link
Member

Varible Scope info is being disabled. Variable
Live Range is being enabled. Debug info generated
for debug code is the same, changes only impact on
optimized code.

Variable Live Range updates variable's location
each time something happens to variable's
liveness. For more information see
https://github.com/dotnet/runtime/blob/master/docs/design/features/variabletracking.md

@BrianBohe
Copy link
Member Author

I will open an issue to develop a baseline/contract/protocol so we can check no one downgrade/reduce/break debug info on optimize code with future changes.

@BrianBohe
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2107 in repo dotnet/runtime

@BrianBohe
Copy link
Member Author

@BruceForstall

@jkotas jkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 24, 2020
@BrianBohe
Copy link
Member Author

@dotnet/jit-contrib

@BrianBohe
Copy link
Member Author

@dotnet-bot Test runtime

@BruceForstall
Copy link
Member

/azp list

@BrianBohe
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2107 in repo dotnet/runtime

@BrianBohe
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2107 in repo dotnet/runtime

@BrianBohe
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2107 in repo dotnet/runtime

@BrianBohe BrianBohe closed this Jan 24, 2020
@BrianBohe BrianBohe reopened this Jan 24, 2020
@BrianBohe
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2107 in repo dotnet/runtime

@BrianBohe
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2107 in repo dotnet/runtime

@BrianBohe
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2107 in repo dotnet/runtime

@BrianBohe
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2107 in repo dotnet/runtime

@BrianBohe BrianBohe closed this Jan 27, 2020
@BrianBohe BrianBohe reopened this Jan 27, 2020
@BrianBohe
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2107 in repo dotnet/runtime

@BrianBohe
Copy link
Member Author

/azp list

@BrianBohe
Copy link
Member Author

These were the previous variable ranges

IL Var Num 1:
[rsi [2A , 107 )]
IL Var Num 2:
[rdi [3D , 107 )]
IL Var Num 3:
[rbx [57 , 1EB )]
IL Var Num 4:
[rbp [BA , 1FB )]
IL Var Num 5:
[rsi [121 , 1DC )]
IL Var Num 6:
[rdi [18B , 1A7 )rdi [1A9 , 1DC )]
IL Var Num 7:
[r14 [18E , 1C8 )r14 [1CB , 1E3 )]
IL Var Num 8:
[rbp [59 , 9A )rbp [9C , A0 )]
IL Var Num 9:
[rax [6E , 6E )]
IL Var Num 10:
[r14 [75 , 75 )r14 [79 , 8D )]
IL Var Num 11:
[r14 [BD , FF )r14 [102 , 107 )]
IL Var Num 12:
[rax [D7 , D7 )]
IL Var Num 13:
[r12 [DE , DE )r12 [E1 , F2 )]
IL Var Num 14:
[rdi [123 , 17A )rdi [17C , 189 )]
IL Var Num 15:
[r14 [133 , 169 )r14 [16C , 17A )]
IL Var Num 16:
[r15 [191 , 1CB )r15 [1CE , 1DC )]

and these are new ones

IL Var Num 1:
[rsi [2A , 107 )]
IL Var Num 2:
[rdi [3D , 107 )]
IL Var Num 3:
[rbx [57 , 1EB )]
IL Var Num 4:
[rbp [BA , 1FB )]
IL Var Num 5:
[rsi [121 , 1DC )]
IL Var Num 6:
[rdi [18B , 1DC )]
IL Var Num 7:
[r14 [18E , 1E3 )]
IL Var Num 8:
[rbp [59 , A0 )]
IL Var Num 9:
[rax [6E , 6E )]
IL Var Num 10:
[r14 [75 , 8D )]
IL Var Num 11:
[r14 [BD , 107 )]
IL Var Num 12:
[rax [D7 , D7 )]
IL Var Num 13:
[r12 [DE , F2 )]
IL Var Num 14:
[rdi [123 , 189 )]
IL Var Num 15:
[r14 [133 , 17A )]
IL Var Num 16:
[r15 [191 , 1DC )]

The complete raw output is here

Copy link
Contributor

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

A few comments that should be changed, but otherwise LGTM

src/coreclr/src/jit/codegencommon.cpp Outdated Show resolved Hide resolved
src/coreclr/src/jit/emit.cpp Outdated Show resolved Hide resolved
src/coreclr/src/jit/emit.cpp Outdated Show resolved Hide resolved
src/coreclr/src/jit/emit.cpp Outdated Show resolved Hide resolved
@BrianBohe
Copy link
Member Author

Thanks @jkotas and @CarolEidt for the suggestions, I have applied them all. How do we proceed now?

@jkotas
Copy link
Member

jkotas commented Feb 24, 2020

System.Private.CoreLib.dll increases from 9183232 bytes to 9367552 bytes, which is a 2% of growth with my changes.

What is this growth with these fixes?

LGTM otherwise.

@BrianBohe
Copy link
Member Author

System.Private.CoreLib.dll increases from 9183232 bytes to 9367552 bytes, which is a 2% of growth with my changes.

What is this growth with these fixes?

LGTM otherwise.
@jkotas, with a Release build:

  • System.Private.CoreLib.dll increases from 9432064 to 9504256 (~ +0.76 %)
  • linuxnonjit.dll increases from 1095680 to 1096192 (~ +0.04 %)
  • superpmi-shim-collector.dll increases from 205312 to 205824 (~ +0.2 %)
  • clrjit.dll decreases from 1213952 to 1211904 (~ -0.1 %)

@BruceForstall
Copy link
Member

@briansull Can you please review this from the JIT perspective?

@BrianBohe
Copy link
Member Author

/azp help

@azure-pipelines
Copy link

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@BrianBohe
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@briansull
Copy link
Contributor

I reviewed the older code (under #ifdef USING_VARIABLE_LIVE_RANGE)
as this change just enables the new method of recording variable live range debug info.

And I give my approval for checkin.

Copy link
Contributor

@briansull briansull 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.

@BrianBohe
Copy link
Member Author

@BruceForstall What should we do with the instances failing?

@BruceForstall
Copy link
Member

@BruceForstall What should we do with the instances failing?

You mean: what about test failures? What kind of failures? It looks like you set them to rerun?

In general, I would rerun until they pass. Unless there are known problems such as documented in #702.

@BrianBohe
Copy link
Member Author

@BruceForstall What should we do with the instances failing?

You mean: what about test failures? What kind of failures? It looks like you set them to rerun?

In general, I would rerun until they pass. Unless there are known problems such as documented in #702.

@BruceForstall
I mean the two pipelines: runtime and runtime (Test crossgen-comparison Linux arm checked). Those have been failing since Friday. The logs showed cancellations due to timeouts, errors publishing artifacts that already exists, and I/O errors due to not finding a file. I don't find those cases listed on the issue you provided.

@BruceForstall
Copy link
Member

@BrianBohe Looks like only crossgen-comparison failed due to infra error. I'd go ahead and merge.

@BrianBohe BrianBohe merged commit 017a64a into dotnet:master Feb 26, 2020
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this pull request Jul 7, 2020
Reverts dotnet#33021, which was a reversion of dotnet#2107.

Fix the IG boundary sensitivity for debug emission.

Allow debug info to be summarized in a way that makes it amenable to analyze
with our jitutils tooling (namely, `jit-analyze`).
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
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.

6 participants