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

[RISC-V][JIT] JIT/CodeGenBringUpTest #84748

Merged
merged 30 commits into from
Apr 27, 2023
Merged

Conversation

clamp03
Copy link
Member

@clamp03 clamp03 commented Apr 13, 2023

JIT/CodeGenBringupTest tests passed

  • Implemented some NYI functions
  • Updated Floating Point Rounding Mode, Compare and JumpCompare
  • Updated Arguments Handling in Prolog
  • Fix some bugs in making constant values
  • A sub task of CoreCLR RISC-V architecture port #84834
  • Some tests failed with CHECKED or RELEASE build.

clamp03 and others added 22 commits April 11, 2023 18:41
- Update Floating Point Compare
- Implement NYI functions
- Implement NYI functions
- Implement NYI functions
- Update genFnPrologCalleeRegArgs
Fix an error in ./JIT/CodeGenBringUpTests/struct16args_d/struct16args_d.sh
Fixed erros in belows
./JIT/CodeGenBringUpTests/Rotate_d/Rotate_d.sh
./JIT/CodeGenBringUpTests/Rotate_do/Rotate_do.sh
./JIT/CodeGenBringUpTests/Rotate_r/Rotate_r.sh
./JIT/CodeGenBringUpTests/Rotate_ro/Rotate_ro.sh
Resolve
```
./JIT/CodeGenBringUpTests/JTrueNeFP_do/JTrueNeFP_do.sh
./JIT/CodeGenBringUpTests/JTrueNeFP_d/JTrueNeFP_d.sh
./JIT/CodeGenBringUpTests/JTrueNeFP_r/JTrueNeFP_r.sh
./JIT/CodeGenBringUpTests/JTrueNeFP_ro/JTrueNeFP_ro.sh
```
Add a missed part for making constant.
In the cases, it is -12.

Resolved
./JIT/CodeGenBringUpTests/NegRMW_do/NegRMW_do.sh
./JIT/CodeGenBringUpTests/NegRMW_ro/NegRMW_ro.sh
TODO-RISCV64: We should optimize constant and address construction.
- Change fcvt to fmv in ins_Copy
- Add rounding mode
- Add Neg for float
- Add rounding mode in float arithmetic
- remove dead code
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 13, 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 13, 2023
@ghost
Copy link

ghost commented Apr 13, 2023

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

Issue Details

Fixed errors in JIT for JIT/CodeGenBringupTest

Author: clamp03
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

Copy link
Member Author

@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.

I need helps and reviews.

src/coreclr/jit/emitriscv64.cpp Show resolved Hide resolved
src/coreclr/jit/emitriscv64.cpp Show resolved Hide resolved
src/coreclr/jit/emitriscv64.cpp Show resolved Hide resolved
src/coreclr/jit/emitriscv64.cpp Show resolved Hide resolved
@@ -1087,45 +1112,56 @@ void emitter::emitIns_I_la(emitAttr size, regNumber reg, ssize_t imm)
{
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 updated a emitIns_I_la which make a constant value. However, I think there is better solution for emitIns_I_la. If you have any idea, please help me! Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be optimized later, when the RISC-V codes are ready to go into the optimizing phase.

Copy link
Member

Choose a reason for hiding this comment

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

For other platforms we have instGen_Set_Reg_To_Imm which is responsible for emitting the instructions to set a register to an arbitrarily sized immediate. However, it lives in CodeGen.

I can see that the emitIns_I_la is used for some contained addresses with large offsets that need to be first materialized into a register. In general, it would be better to avoid containing these addresses in the first place as it misses the point of containment, which is that the parent node can more efficiently handle the codegen of the child node. If the codegen is going to materialize the value into a register anyway, then there is no reason to mark it contained in the first place.

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 see. Next time, I will update emitIns_I_la to instGen_Set_Reg_To_Imm and check containment on RISC-V.
Thank you!

@clamp03
Copy link
Member Author

clamp03 commented Apr 13, 2023

@clamp03
Copy link
Member Author

clamp03 commented Apr 13, 2023

Build failure will be resolved by #82381. Until then, put [WIP] in the title.

@clamp03 clamp03 changed the title [RISC-V][JIT] JIT/CodeGenBringUpTest [WIP][RISC-V][JIT] JIT/CodeGenBringUpTest Apr 13, 2023
- Reverse JCMP cond for reversing jump opt
@clamp03 clamp03 changed the title [WIP][RISC-V][JIT] JIT/CodeGenBringUpTest [RISC-V][JIT] JIT/CodeGenBringUpTest Apr 15, 2023
@alpencolt
Copy link

Could this PR be merged?

@@ -3327,7 +3351,88 @@ void CodeGen::genCodeForCompare(GenTreeOp* tree)
if (varTypeIsFloating(op1Type))
{
assert(tree->OperIs(GT_LT, GT_LE, GT_EQ, GT_NE, GT_GT, GT_GE));
NYI_RISCV64("genCodeForCompare-----unimplemented on RISCV64 yet----");
bool IsUnordered = (tree->gtFlags & GTF_RELOP_NAN_UN) != 0;
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong naming conventions (should be isUnordered), but let's not hold this PR up on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! I will fix in the next PR.

@jakobbotsch jakobbotsch merged commit 4dbaddb into dotnet:main Apr 27, 2023
@clamp03 clamp03 deleted the codegenbringuptest branch April 28, 2023 04:08
@am11 am11 added the arch-riscv Related to the RISC-V architecture label May 13, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 2023
@clamp03 clamp03 self-assigned this Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants