No const arg to temp #48308
Merged
No const arg to temp #48308
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
area-CodeGen-coreclr
sandreenko
reviewed
Feb 16, 2021
Contributor
sandreenko
left a comment
sandreenko
left a comment
There was a problem hiding this comment.
Could you please look at few regressions and see what is causing them?
In order to do this I usually build Core_Root_base, Core_Root_diff and then set COMPlus_NgenDump to the method that I need to get the diffs using different set CORE_ROOT.
The full command in my case would be:
set Core_Root= D:\Sergey\git\runtime\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root_diff
set Complus_NgenDump=methodName
D:\Sergey\git\runtime\artifacts\bin\coreclr\windows.x64.Checked\crossgen.exe /Platform_Assemblies_Paths "D:\Sergey\git\runtime\artifacts\bin\coreclr\windows.x64.Checked\IL" /out "D:\Sergey\git\runtime\artifacts\bin\coreclr\windows.x64.Checked\System.Private.CoreLib.dll" "D:\Sergey\git\runtime\artifacts\bin\coreclr\windows.x64.Checked\IL\System.Private.CoreLib.dll">D:\Sergey\logs\temp\diff.txt
the same for base.
and then compare diff/base in meld/notepad++
src/coreclr/jit/compiler.cpp
Outdated
| } | ||
|
|
||
| BOOL Compiler::IsInvariant(GenTree* tree) | ||
| { |
Contributor
There was a problem hiding this comment.
Do we need this function or GenTree::IsInvariant can be left alone and this one deleted?
Contributor
Author
There was a problem hiding this comment.
The GenTree::IsInvariant wraps a call to this function. I could consolidate it all into one or the other if you think that would be better, I just couldn't get the call to Compiler::IsInvariant to work from within gentree.h.
Contributor
There was a problem hiding this comment.
I have tried it in sandreenko@de99bfd it works well, what issue do you see? Can you cherry-pick this change?
Contributor
Author
There was a problem hiding this comment.
Sure, I don't see any obvious issues. I'll cherry-pick the commit, double check the diffs locally, and push the updated branch if no issues come up in the diffs.
Contributor
Author
There was a problem hiding this comment.
No major change in the diff, pushed the updated branch. Thanks for the help!
src/coreclr/jit/compiler.cpp
Outdated
| optMethodFlags |= OMF_HAS_NULLCHECK; | ||
| } | ||
|
|
||
| BOOL Compiler::IsInvariant(GenTree* tree) |
Contributor
There was a problem hiding this comment.
you can change impIsAddressInLocal to (const GenTree* tree, GenTree** lclVarTreeOut) and declare tree const here as well, so you don't need to cast const away in bool GenTree::IsInvariant() const
| { | ||
| // This can be extended to non-constant nodes, but not to local or indir nodes. | ||
| if (OperIsConst() && ((gtFlags & GTF_REUSE_REG_VAL) != 0)) | ||
| if (IsInvariant() && ((gtFlags & GTF_REUSE_REG_VAL) != 0)) |
Contributor
There was a problem hiding this comment.
looks like it did not get any diffs, it was Total bytes of delta: -2397 (-0.01% of base) before this and after, I wonder if something else is blocking it.
Co-authored-by: Sergey Andreenko <[email protected]>
Co-authored-by: Sergey Andreenko <[email protected]>
Co-authored-by: Sergey Andreenko <[email protected]>
Contributor
Author
Sure, I'll check it and update with more details. |
sandreenko
added
the
enhancement
Contributor
|
/azp run runtime-coreclr jitstress |
Contributor
|
PTAL @AndyAyersMS @dotnet/jit-contrib |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
Author
|
@sandreenko Still looking into the regressions. Merged this branch with master and my updated my local master branch this morning just to make sure everything was up-to-date and re-ran the diffs, I think my first set of numbers may have had a stale build. Here's the new set of diffs: |
Contributor
Author
|
@sandreenko Having a little trouble narrowing down where the regression is. I've been using the command: LogsTo generate the files to compare. When I do a diff of the two files, I'm only seeing differences in addresses, but no real difference in what is being generated. For example, if I look at the code that is generated for
The only difference I'm seeing are addresses used ( Please let me know if you have any advice on what to change to get a better sense of where the regression might be. |
Contributor
|
@alexcovington Note: I have edited your comment to hide the logs under a spoiler, I recommend attaching them as text files in general or it makes it long to scroll between comments 😄 I usually create 2 copies of Core_Root for copies and my command looks like: and it shows the expected diffs: Looks like your changes discovered another issue, opened #48451 for it. It should fix the regressions that are real, the rest look like noise (like LSRA allocates different regs or frame size became smaller and now we can't init it with only SIMD instructions and have to emit some movs). |
Contributor
Author
|
Thanks for the feedback @sandreenko, I didn't know the spoiler feature existed! Very nice.
Sounds good, I'll take another look over just to be sure. Otherwise, let me know if I can add/do anything else for this PR. Only difference I have found so far is the frame size for BaseDiff |
sandreenko
approved these changes
Feb 19, 2021
Contributor
sandreenko
left a comment
sandreenko
left a comment
There was a problem hiding this comment.
LGTM, thanks @alexcovington.
@AndyAyersMS could you please take a look as well?
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR to address #47786. Adds
Compiler::IsInvariantandGenTree::IsInvariantfunctions to check if op is an invariant, changes a few checks to use this function instead of only usingOperIsConst.Here are the library diffs:
Please let me know if I can add/expand on anything above.