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.

Sign up for GitHub

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

Jump to bottom

Disabling disasm checks under certain testing environments #76202

Merged

TIHan merged 10 commits into dotnet:main from TIHan:filecheck-ci-fixes

Sep 27, 2022

Contributor

TIHan commented Sep 26, 2022 •

edited

Loading

Should resolve:

Description

We need to disable disasm checks if we are running under GCStress, TieredPGO, R2R, JITStress.


          Disabling disasm checks under certain testing environments

55d626a

dotnet-issue-labeler bot commented Sep 26, 2022

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ghost assigned TIHan

TIHan added 3 commits

September 26, 2022 11:50


          Add extra linux musl check

9f35945


          Add JitDisasm check back

f94b5ab


          Remove JitDisasm check

65cb4fd

Contributor Author

TIHan commented Sep 26, 2022

@dotnet/jit-contrib @EgorBo This is ready.

danmoseley added the area-CodeGen-coreclr label

ghost commented Sep 26, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Should resolve:

Description

We need to disable disasm checks if we are running under GCStress, TieredPGO, R2R, or on linux-musl(alpine).

Author:	TIHan
Assignees:	TIHan
Labels:	`area-CodeGen-coreclr`
Milestone:	-

TIHan added 2 commits

September 26, 2022 12:22


          Re-enable musl

c35b1db


          Do not run disasm checks under stress modes

95adaf6

Member

markples commented Sep 26, 2022

Are these being disabled because we don't want to test in those scenarios or because it will take more work to test them correctly? In other words, should we open work items to enable them?

markples reviewed

View reviewed changes

src/tests/Common/CLRTest.Jit.targets Outdated

@@ @@ -222,7 +222,10 @@ IF NOT DEFINED DoLink ( @@
                         DependsOnTargets="GetDisasmCheckData">
                   <PropertyGroup>
                     <HasBashDisasmCheck>false</HasBashDisasmCheck>
-                    <HasBashDisasmCheck Condition="'$(HasDisasmCheck)' == 'true' and '$(RuntimeFlavor)' == 'coreclr' and ('$(TargetOS)' == 'Linux' or '$(TargetOS)' == 'OSX') and ('$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64')">true</HasBashDisasmCheck>
+                    <HasBashDisasmCheck Condition="'$(HasDisasmCheck)' == 'true' and '$(RuntimeFlavor)' == 'coreclr' and ('$(TargetOS)' == 'Linux' or '$(TargetOS)' == 'OSX') and ('$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64') and '$(RunCrossGen2)' != 'true'">true</HasBashDisasmCheck>

Member

markples Sep 26, 2022

I think you could fold the RunCrossGen2 check into GetDisasmCheckData and then not duplicate it on the bash/batch sides.

Contributor Author

TIHan Sep 26, 2022

Makes sense, I can also do the setting of GCStressIncompatible/HeapVerifyIncompatible well too.

Member

markples Sep 27, 2022 •

edited

Loading

<EDIT: removed my comment about the Incompatible vars because it was wrong>

markples reviewed

View reviewed changes

src/tests/Common/CLRTest.Jit.targets Outdated

+                    <HasBashDisasmCheck Condition="'$(HasDisasmCheck)' == 'true' and '$(RuntimeFlavor)' == 'coreclr' and ('$(TargetOS)' == 'Linux' or '$(TargetOS)' == 'OSX') and ('$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64') and '$(RunCrossGen2)' != 'true'">true</HasBashDisasmCheck>
+                    <GCStressIncompatible Condition="'$(HasBashDisasmCheck)' == 'true'">true</GCStressIncompatible>
+                    <HeapVerifyIncompatible Condition="'$(HasBashDisasmCheck)' == 'true'">true</HeapVerifyIncompatible>

Member

markples Sep 26, 2022

I am curious to know the restrictions here. I thought these "just" impacted the runtime - do they change the codegen and therefore the disasm output?

Contributor Author

TIHan Sep 26, 2022

I'm not 100% sure - we should probably ask.
@AndyAyersMS - Does GCStress/HeapVerify impact codegen at all?

Member

markples Sep 27, 2022

To be clear - ok to disable and merge - just wondering about the future steps

markples reviewed

View reviewed changes

src/tests/Common/CLRTest.Jit.targets Show resolved Hide resolved

markples reviewed

View reviewed changes

src/tests/Common/CLRTest.Jit.targets Show resolved Hide resolved

markples reviewed

View reviewed changes

src/tests/Common/CLRTest.Jit.targets

-                    <BashDisasmCheckPostCommands Condition="'$(HasBashDisasmCheck)' == 'true'"><![CDATA[
-              if [[ -n $COMPlus_JitDisasm ]]; then
-                @(DisasmCheckFiles -> '  dotnet $CORE_ROOT/SuperFileCheck/SuperFileCheck.dll --csharp "%(Identity)" --allow-unused-prefixes --check-prefixes=CHECK,$(TargetArchitecture.ToUpperInvariant()),$(TargetArchitecture.ToUpperInvariant())-$(TargetOS.ToUpperInvariant()) --dump-input-context 25 --input-file "$(BashDisasmOutputFile)"
+              if [[ ( -z "$COMPlus_JitStress" ) && ( -z "$COMPlus_JitStressRegs" ) && ( -z "$COMPlus_TailcallStress" ) && ( "$COMPlus_TieredPGO" != "1" ) ]]; then

Member

markples Sep 26, 2022

This list is repeated four times, which will be difficult to maintain. For each of bash/batch, the two could be combined to one by defining a variable and using it in the two ifs.

Bash vs batch is difficult because the syntax is different. So perhaps instead of the above suggestion, the condition strings could be separately defined in msbuild variables and included twice in each script. Then those msbuild variables could be defined back-to-back so that it is clear that if you change one you need to change the other.

Contributor Author

TIHan Sep 26, 2022

I'll see if I can clean it up - I'm just so afraid of getting the syntaxes wrong for each of them.

Member

markples Sep 27, 2022

If you have a clean test run, merge without this to unblock outerloop, but please factor this afterwards.

I don't think it's too bad because you already have the script syntax. Something like

<PropertyGroup>
  <DisasmBashCondition>(<![CDATA[( -z "$COMPlus_JitStress" ) && ( -z "$COMPlus_JitStressRegs" ) && ( -z "$COMPlus_TailcallStress" ) && ( "$COMPlus_TieredPGO" != "1" )]]></DisasmBashCondition>
  <DisasmBatchCondition>"%COMPlus_JitStress%"=="" IF "%COMPlus_JitStressRegs%"=="" IF "%COMPlus_TailcallStress%"=="" IF NOT "%COMPlus_TieredPGO%" == "1"</DisasmBatchCondition>
</PropertyGroup>

Then the if lines:

if [[ $(DisasmBashCondition) ]]; then

IF $(DisasmBatchCondition) (

markples reviewed

View reviewed changes

src/tests/Common/CLRTest.Jit.targets Outdated Show resolved Hide resolved

Contributor Author

TIHan commented Sep 26, 2022

Are these being disabled because we don't want to test in those scenarios or because it will take more work to test them correctly? In other words, should we open work items to enable them?

For right now, we should only enable disasm checks for test modes that have predictable codegen.


          Feedback, force a failure

b27a908

Member

markples commented Sep 27, 2022 •

edited

Loading

For right now, we should only enable disasm checks for test modes that have predictable codegen.

I think these are all predictable, except perhaps tiering (not sure if available information can impact the codegen) and the timing and whether the next tier happens can vary by execution in some ways.

But they are different from "normal optimized", which perhaps is what you mean for "right now"?

EDIT - also fine to disable for now even if it should work - I just want to know what is expected, what the next steps are, etc.


          Trying to fix targets

c0e1a8e

Contributor Author

TIHan commented Sep 27, 2022

I just want to know what is expected, what the next steps are, etc.

Unblock outerloop runs
Document limitations on SuperFileCheck and disasm checking(including why we don't run for gcstress/jitstress/etc.)

For this PR, I'm waiting to see a failure on all x64 runs for the single test so I know this worked correctly. If so, I'll fix the test and hopefully we can get this merged in ASAP.

TIHan added 2 commits

September 26, 2022 19:07


          Trying to fix targets

b2a0b5f


          Update Runtime_34937.cs

4bfdbb7

Member

markples commented Sep 27, 2022

...
2. Document limitations on SuperFileCheck and disasm checking(including why we don't run for gcstress/jitstress/etc.)
...

Yes, this is what I'm asking about (and what the future work is to enable scenarios that ought to already work vs add new scenarios)

markples approved these changes

View reviewed changes

Member

markples left a comment

approving but I believe you need to fix the two Incompatible vars by either not putting them in the other Target or modifying it to Return them as I wrote in the comment before it will work fully

tannergooding mentioned this pull request

Support a few "shifted register" operations on Arm64 #75823

Merged

Contributor Author

TIHan commented Sep 27, 2022

I believe you need to fix the two Incompatible vars

I'll make a separate PR for those.

Contributor Author

TIHan commented Sep 27, 2022

CI is clean, failure is unrelated.

TIHan merged commit c6f5267 into dotnet:main

ghost locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr