-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[LoongArch64] Part-2:Add runtime assembly code (*.S) files in nativeaot. #104084
Conversation
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
@shushanhf @huoyaoyuan Could you please review and signoff on this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have limited knowledge on LA64 asm. Here are some points I don't understand or can be improved for consistency.
// by the linker (GD->IE/LE relaxation). | ||
//la.local $a0, \var // | ||
//ld.d \target, $a0, 0 // | ||
//.tlsdesccall \var //TODO-LOONGARCH64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything preventing fixing this TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLSDESC
is a new feature on LoongArch that is not yet supported in LLVM, so that .tlsdesccall
is not defined yet.
LLVM is expected to support TLS in a few months, and we need to wait for LLVM support before fixing it.
Do I need to delete this TODO?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is fine to keep the TODO, but it should explain when it is about. E.g. TODO-LOONGARCH64: Fix once TLSDESC is supported by LLVM (add link to LLVM tracking issue if there is one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sunlijun-610 maybe the LLVM had merged the LA64's TLS patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will add the note of TODO first.
And I will ask the leader of LoongArch LLVM project. If the LA64's TLS patch has been already merged, I will fix it as soon as possible.
Thanks!
@shushanhf Could you please sign-off on this PR? |
OK, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The atomic will be added next comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review the ExceptionHandling.S
src/coreclr/nativeaot/Runtime/unix/unixasmmacrosloongarch64.inc
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/Runtime/unix/unixasmmacrosloongarch64.inc
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/Runtime/unix/unixasmmacrosloongarch64.inc
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This PR is the second part adding nativeaot support on LoongArch64.
It adds runtime assembly code (*.S) files.