Fix isByteReg() assert for x86 #48830
Conversation
The change is similar to the one done in dotnet#46567. If the type if `BYTE`, the use byte regs. Fixes the following failures: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-47307-head-c9ab37658a344842be/System.Linq.Queryable.Tests/console.2ce37104.log?sv=2019-07-07&se=2021-03-10T08%3A39%3A45Z&sr=c&sp=rl&sig=GdS1KlEkJHVcKz7lwpZeszudWYokZpr%2Bt7IGv4c61co%3D https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-47307-head-c9ab37658a344842be/System.Linq.Tests/console.4bc4fd10.log?sv=2019-07-07&se=2021-03-10T08%3A39%3A45Z&sr=c&sp=rl&sig=GdS1KlEkJHVcKz7lwpZeszudWYokZpr%2Bt7IGv4c61co%3D
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
@dotnet/jit-contrib |
| var_types type = varDsc->TypeGet(); | ||
|
|
||
| #ifdef TARGET_X86 | ||
| if (varTypeIsByte(type)) |
There was a problem hiding this comment.
would it be correct to add this logic into allRegs(type) instead?
There was a problem hiding this comment.
That sounds to be appropriate fix, but currently there are too many usage of allRegs(type) and I am not sure how that suggestion would impact other flows. My recommendation is to get this fix and generalize it in future if we find more cases like this.
There was a problem hiding this comment.
Could you please add an assert to allRegs(type) that type != TYP_BYTE to see if any other calls are impacted, if it fails then it should show other existing issues.
| var_types type = varDsc->TypeGet(); | ||
|
|
||
| #ifdef TARGET_X86 | ||
| if (varTypeIsByte(type)) |
There was a problem hiding this comment.
Could you please add an assert to allRegs(type) that type != TYP_BYTE to see if any other calls are impacted, if it fails then it should show other existing issues.
|
@sandreenko - That sounds reasonable. Let me try that out and run jitstressregs jobs. |
This reverts commit 91d9602.
Looks like we hit the assert while compiling System.Private.CoreLib.dll so looks like we need more deeper analysis on right place to generalize it. Here are the failures. The failures happen during register allocation's build interval, so perhaps we use |
ghost
locked as resolved and limited conversation to collaborators
The change is similar to the one done in #46567. If the type if
BYTE, the use byte regs.Fixes the following jitstressregs failures on Windows x86
https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-47307-head-c9ab37658a344842be/System.Linq.Queryable.Tests/console.2ce37104.log?sv=2019-07-07&se=2021-03-10T08%3A39%3A45Z&sr=c&sp=rl&sig=GdS1KlEkJHVcKz7lwpZeszudWYokZpr%2Bt7IGv4c61co%3D
https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-47307-head-c9ab37658a344842be/System.Linq.Tests/console.4bc4fd10.log?sv=2019-07-07&se=2021-03-10T08%3A39%3A45Z&sr=c&sp=rl&sig=GdS1KlEkJHVcKz7lwpZeszudWYokZpr%2Bt7IGv4c61co%3D