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.

Sign up for GitHub

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

Jump to bottom

[RISC-V][JIT] Fix DevDiv_718151 test #88404

Merged
merged 3 commits into from
Jul 18, 2023
Merged

[RISC-V][JIT] Fix DevDiv_718151 test #88404

merged 3 commits into from
Jul 18, 2023

Conversation

clamp03
Copy link
Member

@clamp03 clamp03 commented Jul 5, 2023 •
edited
Loading 

FAILED   - [  22s]./JIT/Regression/JitBlue/DevDiv_718151/DevDiv_718151/DevDiv_718151.sh
               BEGIN EXECUTION
               /work/dotnet/bugfix/artifacts/tests/coreclr/linux.riscv64.Checked/Tests/Core_Root/corerun -p System.Reflection.Metadata.MetadataUpdater.IsSupported=false -p System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization=true DevDiv_718151.dll ''

               Assert failure(PID 628969 [0x000998e9], Thread: 628969 [0x998e9]): Assertion failed 'objChild->isContained()' in 'DevDiv_714266:TestEntryPoint():int' during 'Linear scan register alloc' (IL size 51; hash 0xb6b43c6a; Tier0)

                   File: /home/clamp/Work/dotnet/bugfix/src/coreclr/jit/lsrariscv64.cpp Line: 971
                   Image: /work/dotnet/bugfix/artifacts/tests/coreclr/linux.riscv64.Checked/Tests/Core_Root/corerun

               ./DevDiv_718151.sh: line 448: 628969 Aborted                 (core dumped) $LAUNCHER $ExePath "${CLRTestExecutionArguments[@]}"
               Expected: 100
               Actual: 134
               END EXECUTION - FAILED

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

@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 Jul 5, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 5, 2023
@ghost
Copy link

ghost commented Jul 5, 2023

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

Issue Details

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

Author: clamp03
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution
Milestone: -

@clamp03 clamp03 self-assigned this Jul 5, 2023
@clamp03 clamp03 added the arch-riscv Related to the RISC-V architecture label Jul 5, 2023
SingleAccretion
SingleAccretion reviewed Jul 5, 2023
View reviewed changes
Copy link
Contributor

@SingleAccretion SingleAccretion left a comment

Choose a reason for hiding this comment

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

I think the PUTARG_SPLIT path needs to be updated as well.

src/coreclr/jit/codegenriscv64.cpp Outdated
if (source->OperGet() == GT_LCL_VAR)
// If we have an HFA we can't have any GC pointers,
// if not then the max size for the struct is 16 bytes
if (compiler->IsHfa(layout->GetClassHandle()))
Copy link
Contributor

Choose a reason for hiding this comment

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

There are no HFAs on RISCV, so this checking can be deleted.

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!

@clamp03 clamp03 requested a review from jakobbotsch July 6, 2023 02:05
@jakobbotsch
Copy link
Member

@clamp03 Can you update PUTARG_SPLIT handling too, as suggested by @SingleAccretion above?

@clamp03
Copy link
Member Author

clamp03 commented Jul 6, 2023

@clamp03 Can you update PUTARG_SPLIT handling too, as suggested by @SingleAccretion above?

Oh. I missed the comment. Sorry. I will update then request reviews. Thank you.

alpencolt
alpencolt reviewed Jul 6, 2023
View reviewed changes
src/coreclr/jit/codegenriscv64.cpp Show resolved Hide resolved
src/coreclr/jit/codegenriscv64.cpp Outdated Show resolved Hide resolved
@clamp03
[RISC-V][JIT] Update
45a3d06 
- Update PUTARG_SPLIT
- Remove converting in PUTARG_STK
This was referenced Jul 7, 2023
Closed
Closed
@runfoapp runfoapp bot mentioned this pull request Jul 7, 2023
Closed
@clamp03
Copy link
Member Author

clamp03 commented Jul 10, 2023

@jakobbotsch @SingleAccretion Could you please review? Thank you.

@clamp03
Copy link
Member Author

clamp03 commented Jul 14, 2023

@jakobbotsch @SingleAccretion Could you please review? Thank you.

Please review. Thank you in advance.

@jakobbotsch jakobbotsch reopened this Jul 16, 2023
@jakobbotsch
Copy link
Member

/azp run runtime-coreclr superpmi-replay, runtime-coreclr superpmi-diffs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

jakobbotsch
jakobbotsch reviewed Jul 18, 2023
View reviewed changes
src/coreclr/jit/codegenriscv64.cpp
ClassLayout* layout = source->AsBlk()->GetLayout();
// Pick a register to store intermediate values in for the to-stack
// copy. It must not conflict with addrReg. We try to prefer an
// argument register since those can always use thumb encoding.
Copy link
Member

Choose a reason for hiding this comment

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

"thumb encoding" is an ARM32 thing, but let's not rerun CI for a simple comment change. Feel free to include it in a future change. (Here and below)

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 update later.

@jakobbotsch jakobbotsch merged commit 16dc533 into dotnet:main Jul 18, 2023
134 checks passed
@clamp03 clamp03 deleted the fix_assertion branch August 3, 2023 15:39
@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SingleAccretion SingleAccretion SingleAccretion left review comments

@alpencolt alpencolt alpencolt approved these changes

@jakobbotsch jakobbotsch jakobbotsch approved these changes

Assignees

@clamp03 clamp03

@jakobbotsch jakobbotsch

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@clamp03 @jakobbotsch @alpencolt @SingleAccretion