Skip to content

[mono][2/2] Add SIMD Support for s390x #116779

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
Merged
merged 5 commits into from
Jul 21, 2025
Merged

[mono][2/2] Add SIMD Support for s390x #116779

merged 5 commits into from
Jul 21, 2025

Conversation

saitama951
Copy link
Contributor

This is a follow-up patch to #116669 to add vector support to s390x

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 18, 2025
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 18, 2025
@saitama951
Copy link
Contributor Author

@uweigand @nealef @iii-i , Can you please review the architecture specific code?

@saitama951
Copy link
Contributor Author

cc: @steveisok @giritrivedi @akoeplinger

saitama951
saitama951 commented Jun 18, 2025
View reviewed changes
@saitama951
[mono][2/2] Add SIMD Support for s390x
8dcc633 
This is a followup patch to dotnet#116669 to add vector support to s390x
This was referenced Jun 18, 2025
Closed
Open
Open
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

1 similar comment
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@jkotas jkotas added arch-s390x Related to s390x architecture (unsupported) and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 18, 2025
nealef
nealef reviewed Jun 19, 2025
View reviewed changes
@build-analysis build-analysis bot mentioned this pull request Jun 23, 2025
Open
uweigand
uweigand reviewed Jun 25, 2025
View reviewed changes
@saitama951
Address Review Comments
bc1d701 
* remove SIY_1 (duplicacy)
* remove locgrnle and locghinle use locghiho instead
* reformat whole patch
* remove vflc use vfpso instead
* move common op's to a common ifdef in mini-ops
* remove NEW_INS , it's used no-where
* rewrite the whole logic for Vector conditional ops for floats
* update ANDN with vnc instruction
* add couple of comments
* remove some pseudo op in simd-intrinsics
* add aligned loads and stores
@saitama951
Copy link
Contributor Author

@uweigand Thank you for the in-depth review for the patch, much appreciated. I have addressed most of your review comments

This was referenced Jul 3, 2025
Closed
Open
@risc-vv
Copy link

risc-vv commented Jul 4, 2025

@dotnet/samsung Could you please take a look? These changes may be related to riscv64.

uweigand
uweigand reviewed Jul 8, 2025
View reviewed changes
Copy link
Contributor

@uweigand uweigand left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! A few more comments inline ...

@risc-vv
Copy link

risc-vv commented Jul 9, 2025 •
edited
Loading

RISC-V Release-CLR-QEMU: 9082 / 9112 (99.67%) 
=======================
      passed: 9082
      failed: 2
     skipped: 597
      killed: 28
------------------------
 TOTAL tests: 9709
VIRTUAL time: 37h 28min 3s 692ms
   REAL time: 38min 18s 629ms
=======================

report.xml, report.md, failures.xml, testclr_details.tar.zst

Build information and commands

GIT: bc1d70105ad01d003319b00eb44214b5bcd8c481
CI: d6c9c1ab3a7411819463edc05ded301e89ba586a
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

@saitama951
Adress Review Comments - 2
ca450d2 
* reformat patch - rename opcode from OP_ to OP_S390
* rewrite OP_XCOMPARE and OP_XEXTRACT into one opcode OP_S390_OPXCOMPARE_XEXTRACT
* introduce compare neumonics without setting the condition code
* rewrite OP_VEC_ABS to handle float and integer separately
@saitama951 saitama951 requested a review from uweigand July 18, 2025 14:05
@saitama951 saitama951 force-pushed the simd-patch-2 branch 2 times, most recently from aa9194f to ca08838 Compare July 18, 2025 14:46
uweigand
uweigand reviewed Jul 18, 2025
View reviewed changes
Copy link
Contributor

@uweigand uweigand left a comment

Choose a reason for hiding this comment

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

Thanks! Hopefully final set of comments inline.

@build-analysis build-analysis bot mentioned this pull request Jul 18, 2025
Closed
@saitama951
Address Review Comments - 3
b802d1b 
* remove hard-coded s390_vr16 to allocate a temp_reg instead
* omit move incase ins->sreg and ins->dreg
@saitama951 saitama951 requested a review from uweigand July 19, 2025 08:05
@build-analysis build-analysis bot mentioned this pull request Jul 19, 2025
Open
uweigand
uweigand reviewed Jul 21, 2025
View reviewed changes
Copy link
Contributor

@uweigand uweigand left a comment

Choose a reason for hiding this comment

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

I don't think I can formally approve patches in this repo, but from a s390x architecture perspective this now LGTM. Thanks!

akoeplinger
akoeplinger approved these changes Jul 21, 2025
View reviewed changes
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

This only touches s390x code.

@akoeplinger
Copy link
Member

/ba-g failures on other architectures are not related

@akoeplinger akoeplinger merged commit c6b844e into dotnet:main Jul 21, 2025
72 of 85 checks passed
@jkotas
Copy link
Member

jkotas commented Jul 22, 2025

There are build breaks in CrossAOT_Mono crossaot CI legs on other PRs that seems to have been introduced by this change.

From
https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_apis/build/builds/1101302/logs/472 :

2025-07-21T19:54:52.0156626Z   [ 98%] Building CXX object mono/mini/CMakeFiles/monosgen-objects.dir/mini-llvm-cpp.cpp.o
2025-07-21T19:54:52.3475830Z   /__w/1/s/src/mono/mono/mini/mini-llvm.c:10245:8: error: use of undeclared identifier 'OP_VECTOR_ANDN'
2025-07-21T19:54:52.3478786Z    10245 |                 case OP_VECTOR_ANDN: {
2025-07-21T19:54:52.3481551Z          |                      ^
2025-07-21T19:54:52.3538995Z   /__w/1/s/src/mono/mono/mini/mini-llvm.c:10257:8: error: use of undeclared identifier 'OP_VECTOR_IABS'
2025-07-21T19:54:52.3539699Z    10257 |                 case OP_VECTOR_IABS: {
2025-07-21T19:54:52.3545686Z          |

@saitama951
Copy link
Contributor Author

@jkotas looks like TARGET_X86 is missing from my change in here

https://github.com/saitama951/runtime/blob/main/src/mono/mono/mini/mini-ops.h#L2008-L2015

I'll raise a fix shortly

@saitama951 saitama951 mentioned this pull request Jul 22, 2025
Merged
@saitama951 saitama951 mentioned this pull request Aug 5, 2025
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@akoeplinger akoeplinger akoeplinger approved these changes

@steveisok steveisok Awaiting requested review from steveisok steveisok is a code owner

@vitek-karas vitek-karas Awaiting requested review from vitek-karas vitek-karas is a code owner

+2 more reviewers

@nealef nealef nealef left review comments

@uweigand uweigand uweigand left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
arch-s390x Related to s390x architecture (unsupported) area-Codegen-JIT-mono community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@saitama951 @risc-vv @akoeplinger @jkotas @nealef @uweigand