-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
base: main
Are you sure you want to change the base?
[LoongArch64] Include loongarch64 in supported nativeaot RIDs. #108962
Conversation
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
@shushanhf @MichalStrehovsky @jkotas Could you please review this PR? |
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." /> | ||
|
||
<Error Condition="'$(IlcHostPackagePath)' == '' and '$(RuntimePackagePath)' != '' and ('$(_hostArchitecture)' == 'x64' or '$(_hostArchitecture)' == 'arm64')" |
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.
Does it need adding loongarch64 for Line73?
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.
@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.
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.
Yeah, I don't know what that condition is guarding against, I would just delete the _hostArchitecture
part and see if it breaks anything.
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.
<Error Condition="'$(IlcHostPackagePath)' == '' and '$(RuntimePackagePath)' != '' and ('$(_hostArchitecture)' == 'x64' or '$(_hostArchitecture)' == 'arm64')" | |
<Error Condition="'$(IlcHostPackagePath)' == '' and '$(RuntimePackagePath)' != ''" |
<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." /> |
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 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.
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, then I'll delete them now. Thanks!
Add
loongarch64
insrc/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Publish.targets
.