Skip to content

[RISC-V] Fix GitHub_* tests - #88640

Merged
jakobbotsch merged 4 commits into
dotnet:mainfrom
clamp03:fix_github17585
Jul 16, 2023
Merged

[RISC-V] Fix GitHub_* tests#88640
jakobbotsch merged 4 commits into
dotnet:mainfrom
clamp03:fix_github17585

Conversation

@clamp03

@clamp03clamp03 commented Jul 11, 2023

Copy link
Copy Markdown
Member
  • Fix an assertion in GitHub_17585 by updating DISPATCH STUB
  • Fix a invalid result in GitHub_23147 by updating genJmpMethod in codegenriscv64.cpp
FAILED - [ 20s]./JIT/Regression/JitBlue/GitHub_17585/GitHub_17585/GitHub_17585.sh
BEGIN EXECUTION
/work/dotnet/riscv64/artifacts/tests/coreclr/linux.riscv64.Checked/Tests/Core_Root/corerun -p System.Reflection.Metadata.MetadataUpdater.IsSupported=false -p System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization=true GitHub_17585.dll ''
Assert failure(PID 637342 [0x0009b99e], Thread: 637342 [0x9b99e]): !"AV in DispatchStub at unknown instruction"
File: /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/riscv64/stubs.cpp Line: 876
Image: /work/dotnet/riscv64/artifacts/tests/coreclr/linux.riscv64.Checked/Tests/Core_Root/corerun
./GitHub_17585.sh: line 448: 637342 Aborted (core dumped) $LAUNCHER $ExePath "${CLRTestExecutionArguments[@]}"
Expected: 100
Actual: 134
END EXECUTION - FAILED
FAILED - [ 14s]./JIT/Regression/JitBlue/GitHub_23147/GitHub_23147/GitHub_23147.sh
BEGIN EXECUTION
/work/dotnet/jobs/riscv/bugfix/artifacts/tests/coreclr/linux.riscv64.Checked/Tests/Core_Root/corerun -p System.Reflection.Metadata.MetadataUpdater.IsSupported=false -p System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization=true GitHub_23147.dll ''
jmp_test_f2 FAIL
jmp_test_f3 PASS
jmp_test_f4 PASS
jmp_test_d2 PASS
jmp_test_d3 PASS
jmp_test_d4 PASS
Expected: 100
Actual: 1
END EXECUTION - FAILED

Part of #84834
cc @wscho77@HJLeee@JongHeonChoi@t-mustafin@alpencolt@gbalykov

@ghostghost added community-contribution Indicates that the PR has been added by a community member area-VM-coreclr labels Jul 11, 2023
@clamp03clamp03 self-assigned this Jul 11, 2023
@clamp03clamp03 added the arch-riscv Related to the RISC-V architecture label Jul 11, 2023
@clamp03clamp03 changed the title [RISC-V] Fix GitHub_17585 test failure[RISC-V] Fix GitHub_* testsJul 11, 2023
Comment threadsrc/coreclr/jit/codegenriscv64.cpp Outdated
var_types loadType = TYP_UNDEF;

// NOTE for RISCV64: not supports the HFA.
assert(!varDsc->lvIsHfaRegArg());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HFA is ARM only concept. I think this assert isn't needed.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Removed. Thank you.

gcInfo.gcMarkRegPtrVal(argReg, loadType);

if (compiler->lvaIsMultiregStruct(varDsc, compiler->info.compIsVarArgs))
if (varDsc->GetOtherArgReg() < REG_STK)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's code from LA64, when I made genJmpMethod() implementation, I've took ARM64 version since it's more clear. E.g. what's the meaning of if (varDsc->GetOtherArgReg() < REG_STK)?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Because ARM64 and RISCV have different calling convention. Right, ARM64 seems more clearer, but RISCV follows LA64's. So we need to update like LA64.
GetOtherArgReg is the second register for multi argument register for RISCV and LA64.

In case of ARM64 in the test, it passes two float register arguments using one int register (REG_A0). So compiler->lvaIsMultiregStruct returns false. In method prolog in a callee, it makes two float values from REG_A0 in ARM64.

argRegNext = genRegArgNext(argReg);
argRegNext = varDsc->GetOtherArgReg();

if (emitter::isFloatReg(argRegNext))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

And why special case for float is need?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Because it cannot get the exact type by using varDsc->GetLayout()->GetGCPtrType(1) (only returns TYP_I_IMPL for all primitive and non-gc value type) and GetEmitter()->emitIns_R_S needs the exact type for the arguments.

If you want to update calling convention like ARM64, we can do. However, I think it is better to investigate after RISC-V is stable.

@clamp03

Copy link
Copy Markdown
MemberAuthor

@jkotas@jakobbotsch Could you please review? Thank you so much.

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The virtualcallstubcpu.hpp change looks good.

@jakobbotsch Could you please review the JIT change?

Comment threadsrc/coreclr/jit/codegenriscv64.cpp
Comment threadsrc/coreclr/jit/codegenriscv64.cpp
@jakobbotsch
jakobbotsch merged commit 44c46e1 into dotnet:mainJul 16, 2023
@clamp03
clamp03 deleted the fix_github17585 branch July 16, 2023 23:22
@ghostghost locked as resolved and limited conversation to collaborators Aug 16, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-riscvRelated to the RISC-V architecturearea-VM-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@clamp03@alpencolt@jkotas@jakobbotsch