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

Use REG_INDIRECT_CALL_TARGET_REG for indirect calls on arm64 #101927

Merged
merged 2 commits into from
May 7, 2024

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented May 6, 2024

Fixes: #101896

With #101647 we started using LR as a general purpose register when doing indirect calls and that broke our logic on NativeAOT that detects whether a thread is in an epilog on linux-arm64

Using LR as a GPR is not illegal, just something that we did not do before, so once we see LR (or FP) loaded with some value, the epilog detection logic assumes that we are in an epilog.

Long-term (and assuming that some platforms will never learn how to unwind in epilogs), I think we should encode the epilog ranges. In GC info, I suppose. It will not be trivial as there could be more than one epilog in a method, so encoding them all and then searching through them to answer IsUnwindable question could be a bit tricky. However, it will be a more portable approach.

For now we can have a simpler fix - we do not have to use LR for indirect call targets, so let's not use it.
It seems like REG_INDIRECT_CALL_TARGET_REG is a better fit for this.

@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 6, 2024
Copy link
Contributor

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

@VSadov VSadov requested a review from jakobbotsch May 6, 2024 17:30
@@ -3672,11 +3672,11 @@ void CodeGen::genCallInstruction(GenTreeCall* call)
{
#ifdef TARGET_ARM
// For arm32 we've allocated an internal register to load the target into.
// Loading into lr takes 4 bytes (instead of potentially 2 with another register).
// Loading into IP takes 4 bytes (instead of potentially 2 with another register).
Copy link
Member

Choose a reason for hiding this comment

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

Is there a register called this on arm32?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes (IP/r12 I think), but i think it cannot be loaded in 2byte instr

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I wasn't aware of that alias. R12 would be problematic to use anyway because we use it as an argument register in a bunch of a cases.

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

Thanks! I was not aware that we cannot use LR as a GPR. Is that just a coreclr constraint or a more general property for arm64?

For arm32, we definitely use LR as a scratch register in more cases (REG_SCRATCH is defined as REG_LR). Is that problematic?

@VSadov
Copy link
Member Author

VSadov commented May 6, 2024

Thanks! I was not aware that we cannot use LR as a GPR

That is not illegal. It is just was not done before and at the time when the epilog detection heuristic was introduced, we assumed it will not be needed in the future. We had a small discussion about that.

For arm32, we definitely use LR as a scratch register in more cases.

Not sure how that is handled. Either Arm32 can unwind in epilog (unlikely), or has more complex heuristic that looks for actual epilog patterns.
On arm64 we could get away with just looking for LR/FP loads vs. something that is never in an epilog (calls, branches...).

I've logged a bug to replace this practice with something more robust - like storing the epilog ranges in GC info.
#101932

@VSadov
Copy link
Member Author

VSadov commented May 6, 2024

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@VSadov
Copy link
Member Author

VSadov commented May 7, 2024

All tests are green, including NativeAOT outer loop.

I will add a commit with a comment about current dependency on LR not being a GPR.

@VSadov VSadov merged commit e1c6717 into dotnet:main May 7, 2024
102 of 107 checks passed
@VSadov VSadov deleted the fix101896 branch May 7, 2024 06:20
@VSadov
Copy link
Member Author

VSadov commented May 7, 2024

Thanks!!!

michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
…101927)

* Use REG_INDIRECT_CALL_TARGET_REG for indirect calls on arm64

* Added a comment about NativeAOT dependency at the place wehre we exclude LR from availableIntRegs
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…101927)

* Use REG_INDIRECT_CALL_TARGET_REG for indirect calls on arm64

* Added a comment about NativeAOT dependency at the place wehre we exclude LR from availableIntRegs
@github-actions github-actions bot locked and limited conversation to collaborators Jun 6, 2024
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
Projects
None yet
2 participants