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

JIT: Introduce GT_JTEST and clean up GT_JCMP flags #85242

Merged
merged 4 commits into from
Apr 26, 2023

Conversation

jakobbotsch
Copy link
Member

  • Introduce GT_JTEST to replace GTF_JCMP_TEST
  • Stop encoding JCMP conditions in GenTreeFlags by switching GT_JCMP to a GenTreeOpCC node. This removes GTF_JCMP_EQ and the LoongArch64/RISC-V specific mechanism to communicate condition codes to the backend via gtFlags.

* Introduce GT_JTEST to replace GTF_JCMP_TEST
* Stop encoding JCMP conditions in GenTreeFlags by switching GT_JCMP to
  a GenTreeOpCC node. This removes GTF_JCMP_EQ and the
  LoongArch64/RISC-V specific mechanism to communicate condition codes
  to the backend via gtFlags.
@ghost ghost assigned jakobbotsch Apr 24, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 24, 2023
@ghost
Copy link

ghost commented Apr 24, 2023

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

Issue Details
  • Introduce GT_JTEST to replace GTF_JCMP_TEST
  • Stop encoding JCMP conditions in GenTreeFlags by switching GT_JCMP to a GenTreeOpCC node. This removes GTF_JCMP_EQ and the LoongArch64/RISC-V specific mechanism to communicate condition codes to the backend via gtFlags.
Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

cc @shushanhf @clamp03. This is an alternative to #85217 that stops using GenTreeFlags to communicate the condition and cleans up the representation at the same time.

Also, unrelated to this PR, but I had to apply several patches to get the RISC-V/LoongArch cross-jits to build: https://gist.github.com/jakobbotsch/a11071cec499ef260fbe1b5c85c1b32f
You may want to look into some of these and why they're necessary.

@shushanhf
Copy link
Contributor

cc @shushanhf @clamp03. This is an alternative to #85217 that stops using GenTreeFlags to communicate the condition and cleans up the representation at the same time.

Thansk.

Also, unrelated to this PR, but I had to apply several patches to get the RISC-V/LoongArch cross-jits to build: https://gist.github.com/jakobbotsch/a11071cec499ef260fbe1b5c85c1b32f You may want to look into some of these and why they're necessary.

I will add LA64's image for CI. dotnet/dotnet-buildtools-prereqs-docker#856

@jakobbotsch jakobbotsch marked this pull request as ready for review April 24, 2023 13:14
@jakobbotsch
Copy link
Member Author

jakobbotsch commented Apr 24, 2023

cc @dotnet/jit-contrib PTAL @TIHan -- related to what we have discussed a couple of times. No diffs.

@clamp03 @shushanhf If you can test the LA/RISC-V changes it would be great.

@jakobbotsch jakobbotsch requested a review from TIHan April 24, 2023 13:15
@shushanhf
Copy link
Contributor

cc @dotnet/jit-contrib PTAL @TIHan -- related to what we have discussed a couple of times. No diffs.

@clamp03 @shushanhf If you can test the LA/RISC-V changes it would be great.

OK,I will test it later.

@shushanhf
Copy link
Contributor

cc @dotnet/jit-contrib PTAL @TIHan -- related to what we have discussed a couple of times. No diffs.
@clamp03 @shushanhf If you can test the LA/RISC-V changes it would be great.

OK,I will test it later.

I just tested some cases. I think this is ok.
All the tests will be finished tomorrow morning.

@shushanhf
Copy link
Contributor

cc @dotnet/jit-contrib PTAL @TIHan -- related to what we have discussed a couple of times. No diffs.
@clamp03 @shushanhf If you can test the LA/RISC-V changes it would be great.

OK,I will test it later.

All the tests will be finished tomorrow morning.

@jakobbotsch
For LoongArch64, this is ok.
Thanks

@clamp03
Copy link
Member

clamp03 commented Apr 25, 2023

Thank you. I will check.
CC #84834

Copy link
Member

@clamp03 clamp03 left a comment

Choose a reason for hiding this comment

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

Checked on RISC-V. It works well. Thank you.

@jakobbotsch jakobbotsch merged commit ac2b353 into dotnet:main Apr 26, 2023
@jakobbotsch jakobbotsch deleted the cleanup-jcmp branch April 26, 2023 08:04
@ghost ghost locked as resolved and limited conversation to collaborators May 26, 2023
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.

4 participants