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

Check more patterns for retBuffer. #40340

Merged
merged 2 commits into from
Aug 6, 2020
Merged

Conversation

sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Aug 4, 2020

We force return buffers containing GC pointers to be on the stack and before we make a copy we check if the return buffer is already on the stack. Fix the check by adding LCL_VAR_ADDR, ADD(LCL_VAR_ADDR, CNST) patterns.

Fixes #39947, however, the issue happens because of the several factors:

  1. EnableExtraSuperPmiQueries is a debug only variable that is set during collection;
  2. debug only makeExtraStructQueries is called in checked and it could find a SIMD type when SIMD intrinsics are not used, it would call setUsesSIMDTypes(true) {_usesSIMDTypes = true; } in compiler;
  3. based on the value of _usesSIMDTypes struct promotion could make different decisions, for example, promote struct A {simd16 a; };
  4. lclmorph doesn't replace ADDR(LCL_VAR) as LCL_VAR_ADDR if the lclVar is promoted;
  5. LCL_VAR_ADDR was not recognized by the function changed in this PR and it led to additional VM questions that were failing during release replay.

This PR fixes only the 5th. We could fix the first by:

  1. disable EnableExtraSuperPmiQueries until we need it again;
  2. don't record EnableExtraSuperPmiQueries value during collection (hack void MethodContext::recGetIntConfigValue(const WCHAR* name, int defaultValue, int result));
  3. pass -jitoption force EnableExtraSuperPmiQueries=0 -jit2option force EnableExtraSuperPmiQueries=0 during SPMI replay;
  4. do less in makeExtraStructQueries.

The rest SPMI chk/rel diffs are covered by #39908

We force return buffers containing GC pointers to be on the stack and before we make a copy we check if the return buffer is already on the stack. Fix the check by adding `LCL_VAR_ADDR`, `ADD(LCL_VAR_ADDR, CNST)` patterns.
@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 4, 2020
@sandreenko
Copy link
Contributor Author

PTAL @CarolEidt @BruceForstall @dotnet/jit-contrib

@CarolEidt
Copy link
Contributor

LCL_VAR_ADDR was not recognized by the function changed in that PR

What PR are you referring to?

Regarding how to deal with EnableExtraSuperPmiQueries, I'd prefer option 3: pass -jitoption force EnableExtraSuperPmiQueries=0 -jit2option force EnableExtraSuperPmiQueries=0 during SPMI replay for checked/release diffs. But I'm flexible.

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.

LGTM

@sandreenko
Copy link
Contributor Author

LCL_VAR_ADDR was not recognized by the function changed in that PR

What PR are you referring to?

This one, typo, sorry for the confusion.

Regarding how to deal with EnableExtraSuperPmiQueries, I'd prefer option 3: pass -jitoption force EnableExtraSuperPmiQueries=0 -jit2option force EnableExtraSuperPmiQueries=0 during SPMI replay for checked/release diffs. But I'm flexible.

Agree, I think it is safe and easy. Maybe we will lose some methods during SPMI cleaning phase but the result *.mch files should not show any diffs between chk/rel because of EnableExtraSuperPmiQueries. I will push the change to this PR.

It is set during collection for easier test of struct promotion enhancements but it could let to chk/rel diffs if used during replay.
@BruceForstall
Copy link
Member

While forcing -jitoption force EnableExtraSuperPmiQueries=0 -jit2option force EnableExtraSuperPmiQueries=0 during replay seems expedient, it's also unsettling that enabling extra SuperPMI queries during collection can actually affect codegen. Presumably it could also cause the subsequent questions in the normal codegen to differ, leading to missing questions/answers on replay when NOT setting it.

I wonder if it would be possible to move the "extra" calls to the very end of JITing such that any potential side effects couldn't affect the already-generated code? That is, assuming these calls could even legally be made at that time.

@sandreenko
Copy link
Contributor Author

Presumably it could also cause the subsequent questions in the normal codegen to differ, leading to missing questions/answers on replay when NOT setting it.

Yeah, as I said with this approach we could have some new failures during a cleaning stage and we will have fewer methods in the final mch.

@sandreenko sandreenko merged commit a778b7e into dotnet:master Aug 6, 2020
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
* Check more patterns for retBuffer.

We force return buffers containing GC pointers to be on the stack and before we make a copy we check if the return buffer is already on the stack. Fix the check by adding `LCL_VAR_ADDR`, `ADD(LCL_VAR_ADDR, CNST)` patterns.

* Clear `EnableExtraSuperPmiQueries` during SPMI replay.

It is set during collection for easier test of struct promotion enhancements but it could let to chk/rel diffs if used during replay.
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 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.

SuperPMI checked/release failures
4 participants