Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[RISC-V] coreclr-jit directory #82379

Merged
merged 27 commits into from
Apr 8, 2023
Merged

[RISC-V] coreclr-jit directory #82379

merged 27 commits into from
Apr 8, 2023

Conversation

clamp03
Copy link
Member

@clamp03 clamp03 commented Feb 20, 2023

  • Successfully cross-build for RISC-V.
  • Run A simple application "helloworld"
  • Fail a test in clr.paltest

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 20, 2023
@clamp03 clamp03 changed the title [RISCV-V] coreclr-jit directory [RISC-V] coreclr-jit directory Feb 20, 2023
@clamp03 clamp03 mentioned this pull request Feb 20, 2023
@BruceForstall BruceForstall self-requested a review February 20, 2023 05:14
@BruceForstall
Copy link
Member

@dotnet/jit-contrib

@jkotas jkotas added arch-riscv Related to the RISC-V architecture area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Feb 20, 2023
@ghost
Copy link

ghost commented Feb 20, 2023

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

Issue Details
  • Successfully cross-build for RISC-V.
  • Run A simple application "helloworld"
  • Fail a test in clr.paltest
Author: clamp03
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution, arch-riscv

Milestone: -

@clamp03 clamp03 force-pushed the riscv_jit branch 3 times, most recently from 908aab6 to 54a957f Compare February 22, 2023 03:10
@clamp03 clamp03 force-pushed the riscv_jit branch 2 times, most recently from cc695d6 to a0983f2 Compare February 23, 2023 02:07
@clamp03
Copy link
Member Author

clamp03 commented Feb 23, 2023

@dotnet/jit-contrib Could you review this PR? Actually I don't know why tests failed or cancelled.
If you leave any comment, I will update as possible as I can.
Thank you.

@BruceForstall
Copy link
Member

@clamp03 As you know, this is a very large change and will take a significant amount of time and effort to review. We are discussing internally how to approach it, but no decisions have been made yet.

@clamp03
Copy link
Member Author

clamp03 commented Feb 23, 2023

@BruceForstall Thank you for the comment. I can wait. :)

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

First quick pass.

@clamp03 can you please avoid force-pushing when you address feedback? It makes it very hard for reviewers to see what changed.

@@ -24047,7 +24088,7 @@ void ReturnTypeDesc::InitializeStructReturnType(Compiler* comp,
m_regType[i] = comp->getJitGCType(gcPtrs[i]);
}

#elif defined(TARGET_LOONGARCH64)
#elif defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
assert((structSize >= TARGET_POINTER_SIZE) && (structSize <= (2 * TARGET_POINTER_SIZE)));

uint32_t floatFieldFlags = comp->info.compCompHnd->getLoongArch64PassStructInRegisterFlags(retClsHnd);
Copy link
Member

Choose a reason for hiding this comment

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

Seems a bit odd to call getLoongArch64PassStructInRegisterFlags here, but I guess the ABIs are close enough/equivalent for this to work out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry. It is really odd even if these are very close. I created a new for RISCV64 and pushed a commit in both vm and jit PRs.
Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

This also requires updating the JIT-EE GUID and ThunkInput.txt. See https://github.com/dotnet/runtime/blob/main/docs/project/updating-jitinterface.md for more details.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is ok to leave it unimplemented on the VM side in this PR. Can you introduce a skeleton for this new JIT-EE API? If you need some more help then #78593 is an example PR that introduces a new JIT-EE API.

Copy link
Member Author

Choose a reason for hiding this comment

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

I already added ThunkInput.txt and the other codes into #82381. (*. I missed GUID update.)

If you think that all related codes should be in a PR, then how about reverting the last commit and make a new PR after this coreclr-jit and coreclr-vm (#82381) PR are merged?
If not, I will update GUID in #82381 and so on.
Thank you

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that we cannot merge this PR without coherent JIT-EE implementations/consumptions for all our tooling. I would suggest adding the helper (including the VM side and the rest of the boilerplate) as part of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved coreclr-vm commit to this and updated GUID.
Thank you.

case GT_XOR:
return emitter::isValidUimm11(immVal);
case GT_JCMP:
assert(((parentNode->gtFlags & GTF_JCMP_TST) == 0) ? (immVal == 0) : isPow2(immVal));
Copy link
Member

Choose a reason for hiding this comment

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

Does RISC-V have the equivalent of ARM64's tbnz/tbz instructions?

Choose a reason for hiding this comment

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

Nope. In general, replace TNBZ Rt, #imm, label with SLLI tmp, Rt, 63-imm; BLTZ tmp, label and TBZ with the same but BGEZ. 2 byte instruction plus 4 byte branch instruction.

For imm in 0..10 you could use ANDI with 1<<imm, then BEQZ or BNEZ, which is a 4 byte instruction then a 2 byte branch... But it's the same size and performance anyway

Copy link
Member Author

@clamp03 clamp03 Apr 6, 2023

Choose a reason for hiding this comment

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

@BruceForstall Thank you so much. Oops :)
@brucehoult Thank you so much!!

Copy link
Member

Choose a reason for hiding this comment

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

@brucehoult , that is :)

@brucehoult -- looks like you have lots of RISC-V experience. Maybe we'll see you contribute to the .NET implementation?

Choose a reason for hiding this comment

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

@brucehoult -- looks like you have lots of RISC-V experience.

Since you asked :-) I worked on the CoreCLR to Arm port at Samsung R&D Moscow in 2016, bought one of the first HIFive1 RISC-V boards in Jan 2017, then in early 2018 left Samsung to join RISC-V pioneers SiFive in San Mateo where I worked on runtime, compiler, emulator stuff and also contributed to designing the B and V extensions. (and less so others). You'll find my name in the credits for those extensions, plus the unprivileged ISA manual. Also co-moderator of /r/riscv on Reddit since December 2019, in which time we've grown it from 2500 to 14000 members.

Today was my last day at a New Zealand web dev company I've been working at during COVID. Looking at moving back into the RISC-V world now it's starting to explode. Have VisionFive 2 :-) Currently have ssh access to a 64 core 2.0 GHz OoO (similar to Cortex A72) SG2042 and doing a little vector 0.7.1 asm work on that for someone ... mmmm, tasty.

Copy link
Member

@jakobbotsch jakobbotsch Apr 6, 2023

Choose a reason for hiding this comment

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

Thanks @brucehoult! Good luck in the RISC-V world :)

@clamp03 If there is no direct equivalent of tbnz/tbz then RISC-V should not be setting this GTF_JCMP_TST flag. Indeed, I can see that it is never set by LowerJTrue, so this part of the assert here is just dead code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the assert code in the last patch https://github.com/dotnet/runtime/pull/82379/commits/607dd64ec3dbefa2115317fccc95a6a3df483a14

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of last questions! Thanks for the patience.

I will run some stress jobs just to ensure nothing else seems to be affected.

@jakobbotsch
Copy link
Member

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jakobbotsch jakobbotsch added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 6, 2023
@jakobbotsch jakobbotsch merged commit 6ca0784 into dotnet:main Apr 8, 2023
@jakobbotsch
Copy link
Member

@clamp03 Thanks again for the patience and for addressing all the feedback!

@clamp03
Copy link
Member Author

clamp03 commented Apr 9, 2023

@jakobbotsch Thank you so much. I am really appreciate for your reviews and hard works.

@clamp03 clamp03 deleted the riscv_jit branch April 9, 2023 23:07
@ghost ghost locked as resolved and limited conversation to collaborators May 10, 2023
@clamp03 clamp03 self-assigned this Sep 7, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
arch-riscv Related to the RISC-V architecture area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants