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

fix the regs conflicts within the addResolution for LoongArch64 and RISC-V. #86294

Merged
merged 3 commits into from
May 17, 2023

Conversation

shushanhf
Copy link
Contributor

@shushanhf shushanhf commented May 16, 2023

This PR is part of the issue #69705 to amend the LA's port.

The LoongArch64 is very different with the AArch64 and AMD64.

// For AArch64 and AMD64:                       |  // For LoongArch64
// cmp $reg1, $reg2  <--just compare.           |  the compare and branch can be written in one instruction.
// branch.condition  <--the condition is here.  |  so the comparing registers should be alive for branch.

If insert move before the branch just liking AArch64 or AMD64, there maybe conflicts.

This should be also valid for RISCV64. cc @clamp03

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

ghost commented May 16, 2023

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

Issue Details

This PR is part of the issue #69705 to amend the LA's port.

The LoongArch64 is very different with the AArch64 and AMD64.

// For AArch64 and AMD64:                       |  // For LoongArch64
// cmp $reg1, $reg2  <--just compare.           |  the compare and branch can be written in one instruction.
// branch.condition  <--the condition is here.  |  so the comparing registers should be alive for branch.

If insert move before the branch just liking AArch64 or AMD64, there maybe conflicts.

Author: shushanhf
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@shushanhf
Copy link
Contributor Author

Hi, @jakobbotsch
Could you please review this PR?
Thanks

@jakobbotsch
Copy link
Member

@shushanhf This should be fixed in LSRA instead. It is what this TODO is about:

#if !defined(TARGET_LOONGARCH64) && !defined(TARGET_RISCV64)
// TODO-LOONGARCH64: Take into account that on LA64, the second
// operand of a JCMP can be in a register too.
assert(!lastNode->OperIs(GT_JCMP, GT_JTEST) || lastNode->gtGetOp2()->isContained());
#endif

@shushanhf
Copy link
Contributor Author

@shushanhf This should be fixed in LSRA instead. It is what this TODO is about:

#if !defined(TARGET_LOONGARCH64) && !defined(TARGET_RISCV64)
// TODO-LOONGARCH64: Take into account that on LA64, the second
// operand of a JCMP can be in a register too.
assert(!lastNode->OperIs(GT_JCMP, GT_JTEST) || lastNode->gtGetOp2()->isContained());
#endif

OK, Thanks.
I will amend it for LA.

@shushanhf shushanhf changed the title [LoongArch64] fix the regs conflicts within the addResolution. fix the regs conflicts within the addResolution for LoongArch64 and RISC-V. May 16, 2023
clamp03

This comment was marked as duplicate.

clamp03

This comment was marked as duplicate.

clamp03

This comment was marked as duplicate.

clamp03

This comment was marked as duplicate.

clamp03

This comment was marked as duplicate.

src/coreclr/jit/lsra.cpp Outdated Show resolved Hide resolved
@jakobbotsch
Copy link
Member

Test failure is #86233 that is known by build analysis.

@shushanhf shushanhf deleted the addResolution_cond_branch branch May 18, 2023 00:58
@ghost ghost locked as resolved and limited conversation to collaborators Jun 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

3 participants