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

Ensure the size of Vector<T> takes COMPlus_EnableHWIntrinsic into account #39368

Merged
merged 3 commits into from
Jul 23, 2020

Conversation

tannergooding
Copy link
Member

This replaces #38879, which was saying it had 250 commits

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 15, 2020
@tannergooding tannergooding force-pushed the fix-38841 branch 3 times, most recently from 93d90c9 to 1feb624 Compare July 15, 2020 17:17
@BruceForstall
Copy link
Member

What's the status of this? There are still many failures in the runtime-coreclr jitstress-isas-x86 and runtime-coreclr jitstress-isas-arm pipelines:

https://dev.azure.com/dnceng/public/_build/results?buildId=736993&view=ms.vss-test-web.build-test-results-tab

https://dev.azure.com/dnceng/public/_build/results?buildId=736979&view=ms.vss-test-web.build-test-results-tab

cc @CarolEidt @kunalspathak @echesakovMSFT

@tannergooding
Copy link
Member Author

It looks like the x86 ISA failure is just an issue with Runtime_34587 where something is reporting unsupported when it is expected to be supported.

It resolves the other issues. The ARM failure is an unrelated issue that needs to be fixed as well.

I will try to get a fix up for the Runtime_34587 test so this can be merged

@tannergooding
Copy link
Member Author

Failures look to be unrelated eventpipe failures.

There also looks to still be a failure in Runtime_34587 for the nosimd variant, but I can't get it to repro locally:


Return code:      1
Raw output file:      C:\h\w\95710859\w\ABF209A7\e\JIT\Regression\Reports\JIT.Regression\JitBlue\Runtime_34587\Runtime_34587\Runtime_34587.output.txt
Raw output:
BEGIN EXECUTION
 "C:\h\w\95710859\p\corerun.exe" Runtime_34587.dll 
Supported x86 ISAs:
  AES:           False
  AVX:           False
  AVX2:          False
  BMI1:          True
  BMI2:          True
  FMA:           False
  LZCNT:         True
  PCLMULQDQ:     False
  POPCNT:        True
  SSE:           False
  SSE2:          False
  SSE3:          False
  SSE4.1:        False
  SSE4.2:        False
  SSSE3:         False
Supported x64 ISAs:
  AES.X64:       False
  AVX.X64:       False
  AVX2.X64:      False
  BMI1.X64:      False
  BMI2.X64:      False
  FMA.X64:       False
  LZCNT.X64:     False
  PCLMULQDQ.X64: False
  POPCNT.X64:    False
  SSE.X64:       False
  SSE2.X64:      False
  SSE3.X64:      False
  SSE4.1.X64:    False
  SSE4.2.X64:    False
  SSSE3.X64:     False
Supported Arm ISAs:
  AdvSimd:       False
  Aes:           False
  ArmBase:       False
  Crc32:         False
  Dp:            False
  Rdm:           False
  Sha1:          False
  Sha256:        False
Supported Arm64 ISAs:
  AdvSimd.Arm64: False
  Aes.Arm64:     False
  ArmBase.Arm64: False
  Crc32.Arm64:   False
  Dp.Arm64:      False
  Rdm.Arm64:     False
  Sha1.Arm64:    False
  Sha256.Arm64:  False
Expected: 100
Actual: 0
END EXECUTION - FAILED
FAILED
Test Harness Exitcode is : 1
To run the test:
> set CORE_ROOT=C:\h\w\95710859\p
> C:\h\w\95710859\w\ABF209A7\e\JIT\Regression\JitBlue\Runtime_34587\Runtime_34587\Runtime_34587.cmd
Expected: True
Actual:   False

The log here shows POPCNT is being reported true even though it depends on SSE4.2, so something is misconfigured somewhere outside the VM layer.

@tannergooding
Copy link
Member Author

Ah, it's because POPCNT is a scalar ISA; so it doesn't strictly require SIMD support. That probably needs to be tweaked slightly.

@tannergooding
Copy link
Member Author

@CarolEidt, @echesakovMSFT, @kunalspathak; this should be ready for review/merging and should resolve the x86 issues.

I am still investigating a remaining ARM64 issue and will get a PR up for it separately.

@echesakov
Copy link
Contributor

I am still investigating a remaining ARM64 issue and will get a PR up for it separately.

I will try to take a look tomorrow. Just heads up - there is an issue on Arm64 related to EnableHWIntrinsic=0 (fix in #39753) (in case if you are hitting the same issue)

@tannergooding
Copy link
Member Author

This should still be ready for review/merge. The failures are unrelated eventpipe timeouts that are also failing on other CI.

@kunalspathak
Copy link
Member

        return YMM_REGSIZE_BYTES;

do we need similar check here?


Refers to: src/coreclr/src/jit/compiler.h:8345 in 21a5a2a. [](commit_id = 21a5a2a, deletion_comment = False)

@tannergooding
Copy link
Member Author

tannergooding commented Jul 23, 2020

@kunalspathak, I don't believe so; that is not currently under any check other than "IsReadyToRun"; it also has a comment explaining why it differs from maxSIMDStructBytes:

if (opts.IsReadyToRun())
{
// Return constant instead of maxSIMDStructBytes, as maxSIMDStructBytes performs
// checks that are effected by the current level of instruction set support would
// otherwise cause the highest level of instruction set support to be reported to crossgen2.
// and this api is only ever used as an optimization or assert, so no reporting should
// ever happen.
return YMM_REGSIZE_BYTES;
}

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tannergooding tannergooding merged commit 73f0750 into dotnet:master Jul 23, 2020
@SamMonoRT
Copy link
Member

@imhameed fyi

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
…ount (dotnet#39368)

* Ensure the size of Vector<T> takes COMPlus_EnableHWIntrinsic into account

* Add basic logging to Runtime_34587

* Exclude InstructionSet_POPCNT and InstructionSet_POPCNT_X64 if featureSIMD is disabled
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
@tannergooding tannergooding deleted the fix-38841 branch November 11, 2022 15:30
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.

7 participants