This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Add new CI mode for Intel HW intrinsics #15784
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@dotnet-bot test Windows_NT x64 Debug jitx86hwintrinsic |
|
@dotnet-bot help |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
@dotnet-bot test ci please |
|
Welcome to the dotnet/coreclr Perf help The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
|
@tannergooding Ah, thank you for pointing out. I am not familiar with the CI script, so I just give a try here. Do you think this PR correct? How can I limit these new CI jobs to work with Debug and Checked build? |
|
@dotnet-bot test ci please |
The above command should validate it will generate jobs successfully. It otherwise looks correct.
Do we need/want to do that? These jobs are already outer-loop (they don't run with every PR by default) and getting Release mode validation is likely important (I've already found a couple bugs with the inliner/etc). |
netci.groovy
Outdated
| @@ -86,6 +86,11 @@ class Constants { | |||
| 'tailcallstress' : ['COMPlus_TailcallStress' : '1'], | |||
| 'jitsse2only' : ['COMPlus_EnableAVX' : '0', 'COMPlus_EnableSSE3_4' : '0'], | |||
| 'jitnosimd' : ['COMPlus_FeatureSIMD' : '0'], | |||
| 'jitnox86hwintrinsic' : ['COMPlus_EnableIncompleteISAClass' : '1', 'COMPlus_EnableSSE' : '0' , 'COMPlus_EnableSSE2' : '0' , 'COMPlus_EnableSSE3' : '0' , 'COMPlus_EnableSSSE3' : '0' , 'COMPlus_EnableSSE41' : '0' , 'COMPlus_EnableSSE42' : '0' , 'COMPlus_EnableAVX' : '0' , 'COMPlus_EnableAVX2' : '0' , 'COMPlus_EnableAES' : '0' , 'COMPlus_EnableBMI1' : '0' , 'COMPlus_EnableBMI2' : '0' , 'COMPlus_EnableFMA' : '0' , 'COMPlus_EnableLZCNT' : '0' , 'COMPlus_ EnablePCLMULQDQ' : '0' , 'COMPlus_EnablePOPCNT' : '0'], | |||
There was a problem hiding this comment.
'COMPlus_EnableSSE' : '0' , 'COMPlus_EnableSSE2' : '0'
Just wondering, are these only controlling codegen for HWIntrinsics today? I know the others (where applicable) control codegen more broadly.
There was a problem hiding this comment.
Just wondering, are these only controlling codegen for HWIntrinsics today?
Yes, these two just control HW intrinsics, no impact for the floating-point part.
There was a problem hiding this comment.
Should there be a space in COMPlus_ EnablePCLMULQDQ'?
There was a problem hiding this comment.
@benaadams good catch, thank you!
I see. Thanks! |
netci.groovy
Outdated
| 'jitx86hwintrinsic' : ['COMPlus_EnableIncompleteISAClass' : '1'], | ||
| 'jitx86hwintrinsicsseencoding' : ['COMPlus_EnableIncompleteISAClass' : '1', 'COMPlus_EnableAVX' : '0'], | ||
| 'jitx86hwintrinsicnoavx2' : ['COMPlus_EnableIncompleteISAClass' : '1', 'COMPlus_EnableAVX2' : '0'], | ||
| 'jitx86hwintrinsicnosimd' : ['COMPlus_EnableIncompleteISAClass' : '1', 'COMPlus_FeatureSIMD' : '0'], |
There was a problem hiding this comment.
I think this job will fail until https://github.com/dotnet/coreclr/issues/15641 is resolved
There was a problem hiding this comment.
Nevermind, this is controlling the JIT Flag, rather than the compiler defines.
In either case, I think the consensus was to decouple these in the long run, so I'm not sure we should add this job in particular
There was a problem hiding this comment.
No, the environment variable is different from the compile-flag FEATURE_SIMD.
There was a problem hiding this comment.
so I'm not sure we should add this job in particular
Now, this job tests Vector128/256<T> without the underlying SIMD register support (just struct).
There was a problem hiding this comment.
is that a scenario we need/want to support?
There was a problem hiding this comment.
I encountered failures from this issue...
For example, gtNewMustThrowException has to consider Struct and SIMD both https://github.com/dotnet/coreclr/blob/master/src/jit/gentree.cpp#L17910
There was a problem hiding this comment.
For example, gtNewMustThrowException has to consider Struct and SIMD both
This scenario can be covered by "jitnox86hwintrinsic".
But we have the CI job "nosimd", I just wanted to match it.
There was a problem hiding this comment.
I think that scenario will end up falling out once TYP_SIMD is decoupled from FEATURE_SIMD (as per #15641).
That being said, if there are failure cases until it is decoupled, having this job is probably good.
Could you add a comment indicating as such?
There was a problem hiding this comment.
Could you add a comment indicating as such?
Will do.
netci.groovy
Outdated
| @@ -86,6 +86,11 @@ class Constants { | |||
| 'tailcallstress' : ['COMPlus_TailcallStress' : '1'], | |||
| 'jitsse2only' : ['COMPlus_EnableAVX' : '0', 'COMPlus_EnableSSE3_4' : '0'], | |||
| 'jitnosimd' : ['COMPlus_FeatureSIMD' : '0'], | |||
| 'jitnox86hwintrinsic' : ['COMPlus_EnableIncompleteISAClass' : '1', 'COMPlus_EnableSSE' : '0' , 'COMPlus_EnableSSE2' : '0' , 'COMPlus_EnableSSE3' : '0' , 'COMPlus_EnableSSSE3' : '0' , 'COMPlus_EnableSSE41' : '0' , 'COMPlus_EnableSSE42' : '0' , 'COMPlus_EnableAVX' : '0' , 'COMPlus_EnableAVX2' : '0' , 'COMPlus_EnableAES' : '0' , 'COMPlus_EnableBMI1' : '0' , 'COMPlus_EnableBMI2' : '0' , 'COMPlus_EnableFMA' : '0' , 'COMPlus_EnableLZCNT' : '0' , 'COMPlus_ EnablePCLMULQDQ' : '0' , 'COMPlus_EnablePOPCNT' : '0'], | |||
| 'jitx86hwintrinsic' : ['COMPlus_EnableIncompleteISAClass' : '1'], | |||
There was a problem hiding this comment.
This should probably be named incompletehwintrinsic or something similar. It won't be x86 specific long-term and isn't applicable to intrinsics which are fully implemented.
netci.groovy
Outdated
| @@ -86,6 +86,11 @@ class Constants { | |||
| 'tailcallstress' : ['COMPlus_TailcallStress' : '1'], | |||
| 'jitsse2only' : ['COMPlus_EnableAVX' : '0', 'COMPlus_EnableSSE3_4' : '0'], | |||
| 'jitnosimd' : ['COMPlus_FeatureSIMD' : '0'], | |||
| 'jitnox86hwintrinsic' : ['COMPlus_EnableIncompleteISAClass' : '1', 'COMPlus_EnableSSE' : '0' , 'COMPlus_EnableSSE2' : '0' , 'COMPlus_EnableSSE3' : '0' , 'COMPlus_EnableSSSE3' : '0' , 'COMPlus_EnableSSE41' : '0' , 'COMPlus_EnableSSE42' : '0' , 'COMPlus_EnableAVX' : '0' , 'COMPlus_EnableAVX2' : '0' , 'COMPlus_EnableAES' : '0' , 'COMPlus_EnableBMI1' : '0' , 'COMPlus_EnableBMI2' : '0' , 'COMPlus_EnableFMA' : '0' , 'COMPlus_EnableLZCNT' : '0' , 'COMPlus_ EnablePCLMULQDQ' : '0' , 'COMPlus_EnablePOPCNT' : '0'], | |||
| 'jitx86hwintrinsic' : ['COMPlus_EnableIncompleteISAClass' : '1'], | |||
| 'jitx86hwintrinsicsseencoding' : ['COMPlus_EnableIncompleteISAClass' : '1', 'COMPlus_EnableAVX' : '0'], | |||
There was a problem hiding this comment.
Does disabling AVX also disable AVX2?
Also, would LegacyEncoding be a better name? It seems the Architecture Manual refers to them (most generally) as VEX.128 encoded version, VEX.256 encoded version, and Legacy SSE version
There was a problem hiding this comment.
Does disabling AVX also disable AVX2?
Yes. EnableAVX was introduced by Vector<T>, so it should control AVX and AVX2 both.
Also, would LegacyEncoding be a better name?
No preference, both ok to me.
There was a problem hiding this comment.
Question. Can we have a shortcut for a group of tests? For example, jitx86hwintrinsic can trigger all these new CI jobs.
There was a problem hiding this comment.
Can we have a shortcut for a group of tests?
Yes. I forget the exact syntax. @mmitche might know. Otherwise, I'll look it up in a bit and get back to you.
There was a problem hiding this comment.
Ok, so to confirm COMPlus_SSE=0 will also disable SSE2, SSE3, SSSE3, SSE41, and SSE42, correct? Does it also control AVX and AVX2?
Do we have this documented anywhere yet?
There was a problem hiding this comment.
Ok, so to confirm COMPlus_SSE=0 will also disable SSE2, SSE3, SSSE3, SSE41, and SSE42, correct? Does it also control AVX and AVX2?
No, COMPlus_SSE=0 just control SSE.
There was a problem hiding this comment.
This is probably important as some people may assume the generational nature always hold true.
I think each environment variable should control itself (except AVX) because the generate-to-generation relationship is not always consistent and these knobs are just for debugging.
There was a problem hiding this comment.
these knobs are just for debugging
These knobs will do nothing if set in a Release build of the runtime? I am checking since AVX and SIMD (for example) work in both Release and Debug builds of the runtime.
the generate-to-generation relationship is not always consistent
When is it not consistent? An inconsistency here may cause issues for the non-ISA specific helper functions (such as StaticCast).
It would also be useful to have such inconsistencies documented so that users can reliably know where they need to check (ex: if (Avx.IsSupported) { /* Can I use SSE here? */ } or if (Sse2.IsSupported) { /* Can I use SSE here? */ }
There was a problem hiding this comment.
These knobs will do nothing if set in a Release build of the runtime?
Right, they only work with debug/checked build.
When is it not consistent?
I may need to say "unclear". For example, if SSE can control SSE2, should SSE2 control SSE3/ AES/ AVX?
Originally, most of these variables were introduced for testing IsSupported. A very precise order may be too complex for users.
4creators
reviewed
Jan 9, 2018
netci.groovy
Outdated
| @@ -981,6 +986,11 @@ def static addNonPRTriggers(def job, def branch, def isPR, def architecture, def | |||
| case 'tailcallstress': | |||
| case 'jitsse2only': | |||
| case 'jitnosimd': | |||
| case 'jitnox86hwintrinsic': | |||
| case 'jitx86hwintrinsic': | |||
| case 'jitx86hwintrinsicsseencoding': | |||
There was a problem hiding this comment.
Perhaps to make it shorter: jitx86hwintrinsicssee, in other constants there are only combinations of no and ISA shortcut avx, this would be the same after change here
There was a problem hiding this comment.
Yes, and even we should have no name collisions with other intrinsics as AFAIR Intel is protecting them as trademarks.
|
This has no effect on priority 0 jobs which run in CI with every commit. Can we adopt, perhaps with modifications, #15637? IMO we should run Hardware Intrinsic tests not only in outer loop but with priority 0 tests in CI and locally. At least now all PRs which have #15637 run HW intrinsic tests under CI before being committed. |
|
I meant that we should run basic set of HW intrinsic tests with every commit (my tests are fast - debug ~ 1 min, checked ~ 40 s, release ~ 15s). |
@4creators, I believe we want pri-0 to test the shipping product. Once this goes in, anyone working on intrinsics will be expected to run some test
This will already be the case for any feature complete ISAs (such as LzCnt and PopCnt, which are already doing this). It might be useful to have a helper script to test hardware intrinsics against the various feature flags locally. |
@tannergooding OK I Got it - had wrong understanding of CI test logic and assumtions |
fiigii
changed the title
|
@CarolEidt @BruceForstall PTAL |
|
This LGTM but I will defer to @BruceForstall or @jashook to merge. |
CarolEidt
approved these changes
Jan 10, 2018
|
@dotnet-bot test ci please |
|
Summary of changes: |
|
@jashook thank you for the help. |
|
@fiigii note these jobs will all be priority 1 as you guys seemed to have mentioned and outerloop. As it is the change lgtm |
jashook
approved these changes
Jan 10, 2018
|
Can we get this PR merged? |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
close https://github.com/dotnet/coreclr/issues/15618