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

[LoongArch64] Include loongarch64 in supported nativeaot RIDs. #108962

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@
<Error Condition="'$(DisableUnsupportedError)' != 'true' and '$(OS)' != 'Windows_NT' and '$(_targetOS)' == 'win'"
Text="Cross-OS native compilation is not supported." />

<Error Condition="'$(DisableUnsupportedError)' != 'true' and '$(_targetArchitecture)' != 'x64' and '$(_targetArchitecture)' != 'x86' and '$(_targetArchitecture)' != 'arm64' and '$(_targetArchitecture)' != 'arm'"
<Error Condition="'$(DisableUnsupportedError)' != 'true' and '$(_targetArchitecture)' != 'x64' and '$(_targetArchitecture)' != 'x86' and '$(_targetArchitecture)' != 'arm64' and '$(_targetArchitecture)' != 'arm' and '$(_targetArchitecture)' != 'loongarch64'"
Text="Native compilation does not support targeting $(RuntimeIdentifier) yet." />

<Error Condition="'$(DisableUnsupportedError)' != 'true' and '$(_hostArchitecture)' != 'x64' and '$(_hostArchitecture)' != 'x86' and '$(_hostArchitecture)' != 'arm64' and '$(_hostArchitecture)' != 'arm'"
<Error Condition="'$(DisableUnsupportedError)' != 'true' and '$(_hostArchitecture)' != 'x64' and '$(_hostArchitecture)' != 'x86' and '$(_hostArchitecture)' != 'arm64' and '$(_hostArchitecture)' != 'arm' and '$(_hostArchitecture)' != 'loongarch64'"
Text="Native compilation can run on x64, x86, arm64 and arm hosts only." />
Copy link
Member

Choose a reason for hiding this comment

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

I think we can just delete these lines. At this point the SDK guards this for us. I don't think these errors can be reached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, then I'll delete them now. Thanks!


<Error Condition="'$(IlcHostPackagePath)' == '' and '$(RuntimePackagePath)' != '' and ('$(_hostArchitecture)' == 'x64' or '$(_hostArchitecture)' == 'arm64')"
Copy link
Contributor

@shushanhf shushanhf Oct 17, 2024

Choose a reason for hiding this comment

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

Does it need adding loongarch64 for Line73?

Copy link
Member

Choose a reason for hiding this comment

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

@MichalStrehovsky may be able to confirm, but I think this condition can be simplified without the arch part. In other places, cross means 'host and target arch differ'. So the second part of this condition is not strictly necessary. I have some idea what it is trying to avoid, but even without this part of the condition is not going to degrade the user-experience.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't know what that condition is guarding against, I would just delete the _hostArchitecture part and see if it breaks anything.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Error Condition="'$(IlcHostPackagePath)' == '' and '$(RuntimePackagePath)' != '' and ('$(_hostArchitecture)' == 'x64' or '$(_hostArchitecture)' == 'arm64')"
<Error Condition="'$(IlcHostPackagePath)' == '' and '$(RuntimePackagePath)' != ''"

Expand Down