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

Disable SuperPMI for arm64 apple under jitstress. #53661

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

sandreenko
Copy link
Contributor

The test is failing because we get different answers from VM and SuperPMI dictionary and during replay we CSE different variables and call fgMorph for different trees.

The failure call is happening here:

if (info.compCompHnd->getIntrinsicID(tree->AsCall()->gtCallMethHnd) == CORINFO_INTRINSIC_Object_GetType)

we could have recorded the answer during collection here:

if ((methodFlags & CORINFO_FLG_INTRINSIC) != 0)
{
intrinsicID = info.compCompHnd->getIntrinsicID(method, &mustExpand);
}

but it was not an intrinsic, it was:
// Look for new-style jit intrinsics by name

so if we could protect call in gentree with a similar check we would be fine here, like & GTF_CALL_M_SPECIAL_INTRINSIC == 0 but it does see a benefit of doing it outside of SPMI.

It fails only on arm64 apple because on other platforms we have the failing method in crossgen2 image so JitStress does not affect it.

Also, reenable #12709 that was closed as outdated to see if it was actually fixed.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 3, 2021
@sandreenko sandreenko requested a review from BruceForstall June 3, 2021 06:37
@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sandreenko
Copy link
Contributor Author

PTAL @dotnet/jit-contrib

@sandreenko
Copy link
Contributor Author

could somebody please take a look? it is a simple test disabling, need it to start a stress run in another PR.

@BruceForstall
Copy link
Member

Issue: #53321

@BruceForstall
Copy link
Member

@sandreenko Do you agree that this is some kind of bug that still needs to be fixed, and the test disabling is temporary, only until we get that fix?

@sandreenko
Copy link
Contributor Author

@sandreenko Do you agree that this is some kind of bug that still needs to be fixed, and the test disabling is temporary, only until we get that fix?

Yes and no.
the issue will stop reproducing when we get crossgen2 parity for arm64 apple, but it will still exist (if you disable R2R). This is "Yes" part is to close the issue when crossgen2 is fixed.

"No":
The issue is in SuperPMI that does not fit well for a recording of some not-pure functions of JitEEInterface, because of that we have a clearing phase when we do our SuperPMI collections. I don't think we have a plan to fix this behavior so we don't want to fix it for this case specifically. So SuperPMI fix is out of scope.

I don't see a value to make jit code change for this issue because it will require to distinguish these new-style jit intrinsics from the regular intrinsic for SuperPMI purpose. Nowadays GTF_CALL_M_SPECIAL_INTRINSIC can be set for both so we will need a new flag or something and it means additional compilation time in general.

@sandreenko sandreenko closed this Jun 3, 2021
@sandreenko sandreenko reopened this Jun 3, 2021
@sandreenko sandreenko merged commit 84aadb8 into dotnet:main Jun 3, 2021
@sandreenko sandreenko deleted the disableFailingSPMI branch June 3, 2021 22:24
@BruceForstall
Copy link
Member

I don't understand the specific non-pure-JIT-EE-API issue, but in general, we should work to eliminate the bugs (and I do consider them bugs) that require us to even have a "clean" phase of collection.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 3, 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.

2 participants