Skip to content

Fix crash in SuperPMI PGO apis#47305

Merged
BruceForstall merged 1 commit intodotnet:masterfrom
BruceForstall:FixSpmiPgoApis
Jan 22, 2021
Merged

Fix crash in SuperPMI PGO apis#47305
BruceForstall merged 1 commit intodotnet:masterfrom
BruceForstall:FixSpmiPgoApis

Conversation

@BruceForstall
Copy link
Contributor

The real fix is a trivial fix in the linked list handling
of AllocJitTempBuffer.

The rest of the changes are making the code conform more to SuperPMI
conventions, adding comments, and improving SPMI dumping.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 22, 2021
The real fix is a trivial fix in the linked list handling
of AllocJitTempBuffer.

The rest of the changes are making the code conform more to SuperPMI
conventions, adding comments, and improving SPMI dumping.
@BruceForstall
Copy link
Contributor Author

@kunalspathak @AndyAyersMS PTAL
cc @dotnet/jit-contrib

Copy link
Contributor

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Fix looks good to me. If you agree, can you also update the pipeline schedule to also trigger if there are any chances in src\coreclr\ToolBox\superpmi\*?

@BruceForstall BruceForstall merged commit 2e42668 into dotnet:master Jan 22, 2021
@BruceForstall BruceForstall deleted the FixSpmiPgoApis branch January 22, 2021 18:56
@BruceForstall
Copy link
Contributor Author

If you agree, can you also update the pipeline schedule to also trigger if there are any chances in src\coreclr\ToolBox\superpmi*

I was thinking about this, and I decided not to do that: I don't want to force a re-collection on the potential to just fix a bug that doesn't require a re-collection. So, I'd prefer giving developers more choice and responsibility to "do the right thing" even if that means people might forget to do a collection when it would be required. Hopefully that doesn't bite us too often.

@kunalspathak
Copy link
Contributor

I was thinking about this, and I decided not to do that: I don't want to force a re-collection on the potential to just fix a bug that doesn't require a re-collection. So, I'd prefer giving developers more choice and responsibility to "do the right thing" even if that means people might forget to do a collection when it would be required. Hopefully that doesn't bite us too often.

Sure, so I guess what you are saying is if I just sync your SPMI changes, my existing SPMI collection should work. Makes sense.

@BruceForstall
Copy link
Contributor Author

so I guess what you are saying is if I just sync your SPMI changes, my existing SPMI collection should work.

That's not quite true: I changed the SPMI file format. However, I also changed the JIT/EE GUID, so if you pick up these changes, you need to wait until the triggered SPMI collection is published.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 21, 2021
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Feb 23, 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