-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[RISC-V] coreclr-jit directory #82379
Conversation
clamp03
commented
Feb 20, 2023
- Successfully cross-build for RISC-V.
- Run A simple application "helloworld"
- Fail a test in clr.paltest
@dotnet/jit-contrib |
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue Details
|
908aab6
to
54a957f
Compare
cc695d6
to
a0983f2
Compare
@dotnet/jit-contrib Could you review this PR? Actually I don't know why tests failed or cancelled. |
@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. |
@BruceForstall Thank you for the comment. I can wait. :) |
There was a problem hiding this 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.
- Successfully cross-build for RISC-V. - Run A simple application "helloworld" - Fail a test in clr.paltest
@@ -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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/coreclr/jit/lowerriscv64.cpp
Outdated
case GT_XOR: | ||
return emitter::isValidUimm11(immVal); | ||
case GT_JCMP: | ||
assert(((parentNode->gtFlags & GTF_JCMP_TST) == 0) ? (immVal == 0) : isPow2(immVal)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
Azure Pipelines successfully started running 2 pipeline(s). |
@clamp03 Thanks again for the patience and for addressing all the feedback! |
@jakobbotsch Thank you so much. I am really appreciate for your reviews and hard works. |