-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix AltJit and VectorT ISA specification #113901
Conversation
When running as an AltJit when the generated code will not be used (either because of a mismatched VM, or because RunAltJitCode=0), the JIT overwrites the passed in ISAs. When it does so, it zeros out the VectorT* ISA flags. Instead of this, carry over the existing ISA flags. This should work for same-architecture SPMI replays. I'm not sure if additional logic for specifically setting the VectorT* ISA is required for other scenarios, or if incorporating similar logic to `EEJitManager::SetCpuInfo()` into `Compiler::compCompile()` is required. Adds new JIT debugging helpers `dIsa()` and `dIsaFlags()` to dump out string representations of `CORINFO_InstructionSet` and `CORINFO_InstructionSetFlags`. Fixes dotnet#113869
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (1)
- src/coreclr/jit/compiler.cpp: Language not supported
/azp run runtime-coreclr superpmi-replay-apx |
No pipelines are associated with this pull request. |
/azp run runtime-coreclr superpmi-replay-apx |
No pipelines are associated with this pull request. |
@kunalspathak @dotnet/jit-contrib PTAL |
/azp run runtime-coreclr superpmi-replay-apx |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me, provided it solves the apx related failures highlighted in the original issue, but still might not handle cases that I mentioned in #113869 (comment)
True. I think you need to find the JIT-EE call(s) that lead to failures and implement some kind of altjit-specific cross-arch logic for the mismatched VM case. This change won't help the cross-arch case. Actually, it might be completely broken because a number like "41" might have a completely different interpretation. I guess I should check for mismatched VM for this PR. |
That is, AltJit on same architecture (matching VM) with RunAltJitCode=0.
ok, I fixed the PR to only apply to same-architecture cases. Cross-arch cases still need a fix. |
/azp run runtime-coreclr superpmi-replay-apx |
Azure Pipelines successfully started running 1 pipeline(s). |
/ba-g unrelated failures |
When running as an AltJit when the generated code will not be used (either because of a mismatched VM, or because RunAltJitCode=0), the JIT overwrites the passed in ISAs. When it does so, it zeros out the VectorT* ISA flags.
Instead of this, carry over the existing ISA flags. This should work for same-architecture SPMI replays.
I'm not sure if additional logic for specifically setting the VectorT* ISA is required for other scenarios, or if incorporating similar logic to
EEJitManager::SetCpuInfo()
intoCompiler::compCompile()
is required.Adds new JIT debugging helpers
dIsa()
anddIsaFlags()
to dump out string representations ofCORINFO_InstructionSet
andCORINFO_InstructionSetFlags
.Fixes #113869