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

[mono][jit] Add JIT support for the methods in Vector128<T> on amd64. #86546

Merged
merged 21 commits into from
Jun 13, 2023

Conversation

vargaz
Copy link
Contributor

@vargaz vargaz commented May 21, 2023

Contributes to #86272

@ghost ghost assigned vargaz May 21, 2023
@vargaz vargaz changed the title Amd64 vector t [mono][jit] Add JIT support for the methods in Vector64<T>/Vector128<T> on amd64. May 21, 2023
@vargaz vargaz force-pushed the amd64-vector-t branch 4 times, most recently from 7848094 to d445a8e Compare May 22, 2023 09:23
@vargaz vargaz added the NO-REVIEW Experimental/testing PR, do NOT review it label May 22, 2023
@vargaz vargaz changed the title [mono][jit] Add JIT support for the methods in Vector64<T>/Vector128<T> on amd64. [mono][jit] Add JIT support for the methods in Vector128<T> on amd64. Jun 6, 2023
@vargaz vargaz removed the NO-REVIEW Experimental/testing PR, do NOT review it label Jun 11, 2023
@vargaz vargaz marked this pull request as ready for review June 11, 2023 12:11
@vargaz vargaz requested review from fanyang-mono and jandupej June 11, 2023 12:11
@vargaz
Copy link
Contributor Author

vargaz commented Jun 11, 2023

There are still some test failures on the llvmaot lane.

switch (ins->inst_c1) {
case MONO_TYPE_I1:
case MONO_TYPE_U1:
amd64_sse_pxor_reg_reg (code, SIMD_TEMP_REG, SIMD_TEMP_REG);
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified to:

pxor dreg, dreg
psubb dreg, sreg1

src/mono/mono/mini/mini-amd64.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-amd64.c Outdated Show resolved Hide resolved
src/mono/mono/mini/mini-amd64.c Show resolved Hide resolved
src/mono/mono/mini/mini-amd64.c Show resolved Hide resolved
@vargaz
Copy link
Contributor Author

vargaz commented Jun 13, 2023

Failures look unrelated. Some of the code generated for some methods might not be optimal, but that probably doesn't matter at this point due to other simd codegen shortcomings.

@vargaz vargaz merged commit ac2d3fb into dotnet:main Jun 13, 2023
@vargaz vargaz deleted the amd64-vector-t branch June 13, 2023 22:00
@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 2023
if (vector_size != 128)
return NULL;
#ifdef TARGET_WIN32
return NULL;
Copy link
Member

Choose a reason for hiding this comment

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

@vargaz if I understand this correctly, there is currently no support for Vector128<T> on windows-x64, is that correct?

I am asking this because I noticed System.Runtime.Intrinsics.Tests.Vectors.Vector128Tests.* started failing on runtime-extra-platforms pipeline in the following legs:

  • net8.0-windows-Release-x64-Mono_Release-Windows.11.Amd64.Client.Open
  • net8.0-windows-Release-x64-Mono_Release-Windows.Amd64.Server2022.Open
  • net8.0-windows-Release-x64-Mono_Release-Windows.81.Amd64.Open

which was reported here: #88983
The failures started happening on Jun-14 being the same date when this PR got merged in.

Should we disable the tests? And is there a tracking issue about the missing support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is no support right now. In theory, this PR shouldn't have changed anything on winx64, since the support was disabled previously as well.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. I was puzzled by this as well.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I will continue investigating.

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

Successfully merging this pull request may close these issues.

5 participants