Align LA64 targetpacks with RV64 - #102628

Closed
am11 wants to merge 13 commits into
dotnet:mainfrom
am11:feature/crossgen2/LA64
Closed

Align LA64 targetpacks with RV64#102628
am11 wants to merge 13 commits into
dotnet:mainfrom
am11:feature/crossgen2/LA64

Conversation

@am11

@am11am11 commented May 23, 2024

Copy link
Copy Markdown
Member

Also consolidates <NativeAotSupported definitions.

@ghostghost added the area-ReadyToRun label May 23, 2024
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label May 23, 2024
@am11
am11force-pushed the feature/crossgen2/LA64 branch from 459b546 to dfa88cbCompareMay 23, 2024 21:36
@am11am11 added the arch-riscv Related to the RISC-V architecture label May 23, 2024
@am11
am11 marked this pull request as ready for review May 24, 2024 00:12
shushanhf
shushanhf approved these changes May 24, 2024
View reviewed changes

@shushanhfshushanhf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM for LoongArch64.

Thanks very much!

@build-analysisbuild-analysisBot mentioned this pull request May 24, 2024
Closed
MichalStrehovsky
MichalStrehovsky reviewed May 24, 2024
View reviewed changes
Comment threadeng/Subsets.props Outdated
Comment threadsrc/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj Outdated
@gbalykov

gbalykov commented May 24, 2024

Copy link
Copy Markdown
Member

cc @dotnet/samsung

gbalykov
gbalykov approved these changes May 24, 2024
View reviewed changes

@gbalykovgbalykov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me

@am11
am11force-pushed the feature/crossgen2/LA64 branch from 32d98b7 to 8479358CompareMay 24, 2024 10:00
This was referenced May 24, 2024
Closed
Closed
Closed
Closed
@am11
am11force-pushed the feature/crossgen2/LA64 branch from b40bca0 to 7eec988CompareMay 29, 2024 16:04
@am11

am11 commented May 29, 2024

Copy link
Copy Markdown
MemberAuthor

@MichalStrehovsky, @filipnavara, with #102760 merged the build error is fixed here. This is now publishing linux-arm crossgen2 as an AOT app (instead of R2R), bringing it to the same arm64,x64 plan.

@am11
am11 requested review from jkoritzinsky and jkotasMay 31, 2024 08:04
jkotas
jkotas reviewed Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props Outdated
<_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
<NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
<NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
<NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>

@jkotasjkotasJun 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need '$(CrossBuild)' != 'true' in the condition? I would expect '$(TargetOS)' == '$(HostOS)' to be enough.

@am11am11Jun 2, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This part of the condition was adapted from main

runtime/src/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj

Lines 13 to 14 in cb71b60

<!-- Publish crossgen2 as a single-file app on native-OS builds. Cross-OS NativeAOT compilation is not supported yet -->
<NativeAotSupportedCondition="'$(CrossBuild)' == 'true' and '$(TargetOS)' != '$(HostOS)'">false</NativeAotSupported>

e.g. x64 binary running on arm64 OS is supported (as long as the host and target OS were same).

@jkotasjkotasJun 2, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

x64 binary running on arm64 OS is supported (as long as the host and target OS were same).

This is situation should be covered by '$(TargetOS)' != '$(HostOS)' condition. I do not understand why we also check for CrossBuild in the condition.

jkotas
jkotas reviewed Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
<_NativeAotSupportedOS Condition="'$(TargetOS)' == 'windows' or '$(TargetOS)' == 'linux' or '$(TargetOS)' == 'osx' or '$(TargetOS)' == 'maccatalyst' or '$(TargetOS)' == 'iossimulator' or '$(TargetOS)' == 'ios' or '$(TargetOS)' == 'tvossimulator' or '$(TargetOS)' == 'tvos' or '$(TargetOS)' == 'freebsd'">true</_NativeAotSupportedOS>
<_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
<NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
<NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>

@jkotasjkotasJun 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/blob/main/src/native/managed/compile-native.proj#L18-L28 is another place that checks for whether it is possible to use native AOT to produce binaries compiler by the repo. Can we use this property there as well?

jkotas
jkotas reviewed Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props Outdated
<_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
<NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
<NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
<NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>

@jkotasjkotasJun 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure whether NativeAotCanExecuteOnHost is the right name for this property. We always have ilc that can execute on host for all host OSes. The problem is that we do not always have a working host/target build environment.

Would something like UseNativeAotForComponents be a better name?

am11
am11 commented Jun 2, 2024
View reviewed changes
Comment threadsrc/native/managed/compile-native.proj

<!-- some special kinds of runtime builds need extra NativeAOT flags -->
<PropertyGroup>
<SysRoot Condition="'$(CrossBuild)' == 'true' and '$(HostOS)' != 'windows'">$(ROOTFS_DIR)</SysRoot>

@am11am11Jun 2, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

These SysRoot setup can also be brought to the same plan as ILCompiler.csproj and crossgen2.csproj (better deduplicate all three using init-compiler.sh thingy). I have a feeling it will take some digging which is why @lambdageek chose this approach, but worth a try (in a follow-up PR). 🙂

@build-analysisbuild-analysisBot mentioned this pull request Jun 2, 2024
Closed
@MichalStrehovsky

MichalStrehovsky commented Jun 3, 2024

Copy link
Copy Markdown
Member

am11 requested review from jkoritzinsky, jkotas and MichalStrehovsky 3 days ago

I think it would help reviewing if you could split off the native AOT change to a separate PR. I don't know about target packs or RV64 and cannot review or sign off on that.

I (and probably others) often skim commit histories and PRs to see what's going on and skip stuff with titles that are not related to my areas - a PR named "Align LA64 targetpacks" that spends most lines of the diff on something unrelated breaks such workflows.

@am11am11 closed this Jun 3, 2024
@am11

am11 commented Jun 3, 2024

Copy link
Copy Markdown
MemberAuthor

@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.

@MichalStrehovsky

MichalStrehovsky commented Jun 3, 2024

Copy link
Copy Markdown
Member

@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.

Sorry, it was not clear who would be responsible for getting this merged since it touched multiple things owned/worked on by different teams. I often comment on PRs without an intention of taking the full responsibility of merging it - whoever merged it is the one who's ultimately responsible even if it was authored by you. It was not clear to me who could merge a PR in this shape (I guess Jan often gets to volunteer himself for these, but I don't like dumping all work on Jan). Since you asked me specifically to take a look, my feedback is really that I won't take responsibility for targeting pack changes and we're going to be blocked on finding whoever is qualified (I don't know who), unless the native AOT change is separated out.

@jkotas

jkotas commented Jun 3, 2024

Copy link
Copy Markdown
Member

I know that it is tempting to make multiple somewhat unrelated change in the same PR. I have done it myself many times, and sometimes had to revert it or split into multiple PRs to make review and approval easier.

@am11 Please take @MichalStrehovsky comment only as a suggestion. If you prefer to keep both changes in one PR, that's ok - it is still manageable. We appreciate your contributions.

@LuckyXu-HFLuckyXu-HF mentioned this pull request Jun 11, 2024
Closed
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Jul 4, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas left review comments
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovsky
+5 more reviewers
@tomeksowitomeksowitomeksowi approved these changes
@clamp03clamp03clamp03 approved these changes
@gbalykovgbalykovgbalykov approved these changes
@shushanhfshushanhfshushanhf approved these changes
@BajtazarBajtazarBajtazar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

arch-loongarch64arch-riscvRelated to the RISC-V architecturearea-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@am11@gbalykov@MichalStrehovsky@jkotas@tomeksowi@clamp03@shushanhf@Bajtazar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Align LA64 targetpacks with RV64 - #102628
Closed
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Closed
Align LA64 targetpacks with RV64#102628
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Conversation
@am11
@am11am11
commented
May 23, 2024

Copy link


Copy Markdown

Member
Also consolidates <NativeAotSupported definitions.
@ghostghost
added
the 
area-ReadyToRun

label
May 23, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 23, 2024
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
459b546 to
dfa88cbCompareMay 23, 2024 21:36
@am11am11
added
the 
arch-riscv
Related to the RISC-V architecture
label
May 23, 2024
@am11
am11
marked this pull request as ready for review
May 24, 2024 00:12
shushanhf
shushanhf
approved these changes
May 24, 2024
View reviewed changes
@shushanhfshushanhf
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM for LoongArch64.
Thanks very much!
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Closed

MichalStrehovsky
MichalStrehovsky
reviewed
May 24, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

Comment threadsrc/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj
Outdated

@gbalykov
gbalykov
commented
May 24, 2024

Copy link


Copy Markdown

Member
cc @dotnet/samsung
gbalykov
gbalykov
approved these changes
May 24, 2024
View reviewed changes
@gbalykovgbalykov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good to me
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
32d98b7 to
8479358CompareMay 24, 2024 10:00

This was referenced May 24, 2024

Closed


Closed


Closed


Closed

@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
b40bca0 to
7eec988CompareMay 29, 2024 16:04
@am11
am11
commented
May 29, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, @filipnavara, with #102760 merged the build error is fixed here. This is now publishing linux-arm crossgen2 as an AOT app (instead of R2R), bringing it to the same arm64,x64 plan.
@am11
am11
requested review from
jkoritzinsky and jkotasMay 31, 2024 08:04
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why do we need '$(CrossBuild)' != 'true' in the condition? I would expect '$(TargetOS)' == '$(HostOS)' to be enough.
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This part of the condition was adapted from main 
runtime/src/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj

Lines 13 to 14
in
cb71b60
<!-- Publish crossgen2 as a single-file app on native-OS builds. Cross-OS NativeAOT compilation is not supported yet -->
 <NativeAotSupportedCondition="'$(CrossBuild)' == 'true' and '$(TargetOS)' != '$(HostOS)'">false</NativeAotSupported> 
e.g. x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
@jkotasjkotasJun 2, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
This is situation should be covered by '$(TargetOS)' != '$(HostOS)' condition. I do not understand why we also check for CrossBuild in the condition.
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
 <_NativeAotSupportedOS Condition="'$(TargetOS)' == 'windows' or '$(TargetOS)' == 'linux' or '$(TargetOS)' == 'osx' or '$(TargetOS)' == 'maccatalyst' or '$(TargetOS)' == 'iossimulator' or '$(TargetOS)' == 'ios' or '$(TargetOS)' == 'tvossimulator' or '$(TargetOS)' == 'tvos' or '$(TargetOS)' == 'freebsd'">true</_NativeAotSupportedOS>
 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/blob/main/src/native/managed/compile-native.proj#L18-L28 is another place that checks for whether it is possible to use native AOT to produce binaries compiler by the repo. Can we use this property there as well?
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I am not sure whether NativeAotCanExecuteOnHost is the right name for this property. We always have ilc that can execute on host for all host OSes. The problem is that we do not always have a working host/target build environment.
Would something like UseNativeAotForComponents be a better name?
am11
am11
commented
Jun 2, 2024
View reviewed changes
Comment threadsrc/native/managed/compile-native.proj

 <!-- some special kinds of runtime builds need extra NativeAOT flags -->
 <PropertyGroup>
 <SysRoot Condition="'$(CrossBuild)' == 'true' and '$(HostOS)' != 'windows'">$(ROOTFS_DIR)</SysRoot>
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

These SysRoot setup can also be brought to the same plan as ILCompiler.csproj and crossgen2.csproj (better deduplicate all three using init-compiler.sh thingy). I have a feeling it will take some digging which is why @lambdageek chose this approach, but worth a try (in a follow-up PR). 🙂
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 2, 2024

Closed

@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
am11 requested review from jkoritzinsky, jkotas and MichalStrehovsky 3 days ago
I think it would help reviewing if you could split off the native AOT change to a separate PR. I don't know about target packs or RV64 and cannot review or sign off on that.
I (and probably others) often skim commit histories and PRs to see what's going on and skip stuff with titles that are not related to my areas - a PR named "Align LA64 targetpacks" that spends most lines of the diff on something unrelated breaks such workflows.
@am11am11
closed this
Jun 3, 2024
@am11
am11
commented
Jun 3, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
Sorry, it was not clear who would be responsible for getting this merged since it touched multiple things owned/worked on by different teams. I often comment on PRs without an intention of taking the full responsibility of merging it - whoever merged it is the one who's ultimately responsible even if it was authored by you. It was not clear to me who could merge a PR in this shape (I guess Jan often gets to volunteer himself for these, but I don't like dumping all work on Jan). Since you asked me specifically to take a look, my feedback is really that I won't take responsibility for targeting pack changes and we're going to be blocked on finding whoever is qualified (I don't know who), unless the native AOT change is separated out.
@jkotas
jkotas
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
I know that it is tempting to make multiple somewhat unrelated change in the same PR. I have done it myself many times, and sometimes had to revert it or split into multiple PRs to make review and approval easier.
@am11 Please take @MichalStrehovsky comment only as a suggestion. If you prefer to keep both changes in one PR, that's ok - it is still manageable. We appreciate your contributions.
@LuckyXu-HFLuckyXu-HF
mentioned this pull request
Jun 11, 2024
 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 4, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovsky

+5 more reviewers

@tomeksowitomeksowitomeksowi approved these changes
@clamp03clamp03clamp03 approved these changes
@gbalykovgbalykovgbalykov approved these changes
@shushanhfshushanhfshushanhf approved these changes
@BajtazarBajtazarBajtazar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

arch-loongarch64arch-riscvRelated to the RISC-V architecturearea-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@am11@gbalykov@MichalStrehovsky@jkotas@tomeksowi@clamp03@shushanhf@Bajtazar

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Align LA64 targetpacks with RV64 - #102628
Closed
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Closed
Align LA64 targetpacks with RV64#102628
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Conversation
@am11
@am11am11
commented
May 23, 2024

Copy link


Copy Markdown

Member
Also consolidates <NativeAotSupported definitions.
@ghostghost
added
the 
area-ReadyToRun

label
May 23, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 23, 2024
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
459b546 to
dfa88cbCompareMay 23, 2024 21:36
@am11am11
added
the 
arch-riscv
Related to the RISC-V architecture
label
May 23, 2024
@am11
am11
marked this pull request as ready for review
May 24, 2024 00:12
shushanhf
shushanhf
approved these changes
May 24, 2024
View reviewed changes
@shushanhfshushanhf
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM for LoongArch64.
Thanks very much!
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Closed

MichalStrehovsky
MichalStrehovsky
reviewed
May 24, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

Comment threadsrc/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj
Outdated

@gbalykov
gbalykov
commented
May 24, 2024

Copy link


Copy Markdown

Member
cc @dotnet/samsung
gbalykov
gbalykov
approved these changes
May 24, 2024
View reviewed changes
@gbalykovgbalykov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good to me
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
32d98b7 to
8479358CompareMay 24, 2024 10:00

This was referenced May 24, 2024

Closed


Closed


Closed


Closed

@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
b40bca0 to
7eec988CompareMay 29, 2024 16:04
@am11
am11
commented
May 29, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, @filipnavara, with #102760 merged the build error is fixed here. This is now publishing linux-arm crossgen2 as an AOT app (instead of R2R), bringing it to the same arm64,x64 plan.
@am11
am11
requested review from
jkoritzinsky and jkotasMay 31, 2024 08:04
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why do we need '$(CrossBuild)' != 'true' in the condition? I would expect '$(TargetOS)' == '$(HostOS)' to be enough.
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This part of the condition was adapted from main 
runtime/src/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj

Lines 13 to 14
in
cb71b60
<!-- Publish crossgen2 as a single-file app on native-OS builds. Cross-OS NativeAOT compilation is not supported yet -->
 <NativeAotSupportedCondition="'$(CrossBuild)' == 'true' and '$(TargetOS)' != '$(HostOS)'">false</NativeAotSupported> 
e.g. x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
@jkotasjkotasJun 2, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
This is situation should be covered by '$(TargetOS)' != '$(HostOS)' condition. I do not understand why we also check for CrossBuild in the condition.
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
 <_NativeAotSupportedOS Condition="'$(TargetOS)' == 'windows' or '$(TargetOS)' == 'linux' or '$(TargetOS)' == 'osx' or '$(TargetOS)' == 'maccatalyst' or '$(TargetOS)' == 'iossimulator' or '$(TargetOS)' == 'ios' or '$(TargetOS)' == 'tvossimulator' or '$(TargetOS)' == 'tvos' or '$(TargetOS)' == 'freebsd'">true</_NativeAotSupportedOS>
 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/blob/main/src/native/managed/compile-native.proj#L18-L28 is another place that checks for whether it is possible to use native AOT to produce binaries compiler by the repo. Can we use this property there as well?
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I am not sure whether NativeAotCanExecuteOnHost is the right name for this property. We always have ilc that can execute on host for all host OSes. The problem is that we do not always have a working host/target build environment.
Would something like UseNativeAotForComponents be a better name?
am11
am11
commented
Jun 2, 2024
View reviewed changes
Comment threadsrc/native/managed/compile-native.proj

 <!-- some special kinds of runtime builds need extra NativeAOT flags -->
 <PropertyGroup>
 <SysRoot Condition="'$(CrossBuild)' == 'true' and '$(HostOS)' != 'windows'">$(ROOTFS_DIR)</SysRoot>
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

These SysRoot setup can also be brought to the same plan as ILCompiler.csproj and crossgen2.csproj (better deduplicate all three using init-compiler.sh thingy). I have a feeling it will take some digging which is why @lambdageek chose this approach, but worth a try (in a follow-up PR). 🙂
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 2, 2024

Closed

@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
am11 requested review from jkoritzinsky, jkotas and MichalStrehovsky 3 days ago
I think it would help reviewing if you could split off the native AOT change to a separate PR. I don't know about target packs or RV64 and cannot review or sign off on that.
I (and probably others) often skim commit histories and PRs to see what's going on and skip stuff with titles that are not related to my areas - a PR named "Align LA64 targetpacks" that spends most lines of the diff on something unrelated breaks such workflows.
@am11am11
closed this
Jun 3, 2024
@am11
am11
commented
Jun 3, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
Sorry, it was not clear who would be responsible for getting this merged since it touched multiple things owned/worked on by different teams. I often comment on PRs without an intention of taking the full responsibility of merging it - whoever merged it is the one who's ultimately responsible even if it was authored by you. It was not clear to me who could merge a PR in this shape (I guess Jan often gets to volunteer himself for these, but I don't like dumping all work on Jan). Since you asked me specifically to take a look, my feedback is really that I won't take responsibility for targeting pack changes and we're going to be blocked on finding whoever is qualified (I don't know who), unless the native AOT change is separated out.
@jkotas
jkotas
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
I know that it is tempting to make multiple somewhat unrelated change in the same PR. I have done it myself many times, and sometimes had to revert it or split into multiple PRs to make review and approval easier.
@am11 Please take @MichalStrehovsky comment only as a suggestion. If you prefer to keep both changes in one PR, that's ok - it is still manageable. We appreciate your contributions.
@LuckyXu-HFLuckyXu-HF
mentioned this pull request
Jun 11, 2024
 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 4, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovsky

+5 more reviewers

@tomeksowitomeksowitomeksowi approved these changes
@clamp03clamp03clamp03 approved these changes
@gbalykovgbalykovgbalykov approved these changes
@shushanhfshushanhfshushanhf approved these changes
@BajtazarBajtazarBajtazar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

arch-loongarch64arch-riscvRelated to the RISC-V architecturearea-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@am11@gbalykov@MichalStrehovsky@jkotas@tomeksowi@clamp03@shushanhf@Bajtazar

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Align LA64 targetpacks with RV64 - #102628
Closed
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Closed
Align LA64 targetpacks with RV64#102628
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Conversation
@am11
@am11am11
commented
May 23, 2024

Copy link


Copy Markdown

Member
Also consolidates <NativeAotSupported definitions.
@ghostghost
added
the 
area-ReadyToRun

label
May 23, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 23, 2024
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
459b546 to
dfa88cbCompareMay 23, 2024 21:36
@am11am11
added
the 
arch-riscv
Related to the RISC-V architecture
label
May 23, 2024
@am11
am11
marked this pull request as ready for review
May 24, 2024 00:12
shushanhf
shushanhf
approved these changes
May 24, 2024
View reviewed changes
@shushanhfshushanhf
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM for LoongArch64.
Thanks very much!
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Closed

MichalStrehovsky
MichalStrehovsky
reviewed
May 24, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

Comment threadsrc/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj
Outdated

@gbalykov
gbalykov
commented
May 24, 2024

Copy link


Copy Markdown

Member
cc @dotnet/samsung
gbalykov
gbalykov
approved these changes
May 24, 2024
View reviewed changes
@gbalykovgbalykov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good to me
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
32d98b7 to
8479358CompareMay 24, 2024 10:00

This was referenced May 24, 2024

Closed


Closed


Closed


Closed

@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
b40bca0 to
7eec988CompareMay 29, 2024 16:04
@am11
am11
commented
May 29, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, @filipnavara, with #102760 merged the build error is fixed here. This is now publishing linux-arm crossgen2 as an AOT app (instead of R2R), bringing it to the same arm64,x64 plan.
@am11
am11
requested review from
jkoritzinsky and jkotasMay 31, 2024 08:04
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why do we need '$(CrossBuild)' != 'true' in the condition? I would expect '$(TargetOS)' == '$(HostOS)' to be enough.
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This part of the condition was adapted from main 
runtime/src/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj

Lines 13 to 14
in
cb71b60
<!-- Publish crossgen2 as a single-file app on native-OS builds. Cross-OS NativeAOT compilation is not supported yet -->
 <NativeAotSupportedCondition="'$(CrossBuild)' == 'true' and '$(TargetOS)' != '$(HostOS)'">false</NativeAotSupported> 
e.g. x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
@jkotasjkotasJun 2, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
This is situation should be covered by '$(TargetOS)' != '$(HostOS)' condition. I do not understand why we also check for CrossBuild in the condition.
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
 <_NativeAotSupportedOS Condition="'$(TargetOS)' == 'windows' or '$(TargetOS)' == 'linux' or '$(TargetOS)' == 'osx' or '$(TargetOS)' == 'maccatalyst' or '$(TargetOS)' == 'iossimulator' or '$(TargetOS)' == 'ios' or '$(TargetOS)' == 'tvossimulator' or '$(TargetOS)' == 'tvos' or '$(TargetOS)' == 'freebsd'">true</_NativeAotSupportedOS>
 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/blob/main/src/native/managed/compile-native.proj#L18-L28 is another place that checks for whether it is possible to use native AOT to produce binaries compiler by the repo. Can we use this property there as well?
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I am not sure whether NativeAotCanExecuteOnHost is the right name for this property. We always have ilc that can execute on host for all host OSes. The problem is that we do not always have a working host/target build environment.
Would something like UseNativeAotForComponents be a better name?
am11
am11
commented
Jun 2, 2024
View reviewed changes
Comment threadsrc/native/managed/compile-native.proj

 <!-- some special kinds of runtime builds need extra NativeAOT flags -->
 <PropertyGroup>
 <SysRoot Condition="'$(CrossBuild)' == 'true' and '$(HostOS)' != 'windows'">$(ROOTFS_DIR)</SysRoot>
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

These SysRoot setup can also be brought to the same plan as ILCompiler.csproj and crossgen2.csproj (better deduplicate all three using init-compiler.sh thingy). I have a feeling it will take some digging which is why @lambdageek chose this approach, but worth a try (in a follow-up PR). 🙂
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 2, 2024

Closed

@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
am11 requested review from jkoritzinsky, jkotas and MichalStrehovsky 3 days ago
I think it would help reviewing if you could split off the native AOT change to a separate PR. I don't know about target packs or RV64 and cannot review or sign off on that.
I (and probably others) often skim commit histories and PRs to see what's going on and skip stuff with titles that are not related to my areas - a PR named "Align LA64 targetpacks" that spends most lines of the diff on something unrelated breaks such workflows.
@am11am11
closed this
Jun 3, 2024
@am11
am11
commented
Jun 3, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
Sorry, it was not clear who would be responsible for getting this merged since it touched multiple things owned/worked on by different teams. I often comment on PRs without an intention of taking the full responsibility of merging it - whoever merged it is the one who's ultimately responsible even if it was authored by you. It was not clear to me who could merge a PR in this shape (I guess Jan often gets to volunteer himself for these, but I don't like dumping all work on Jan). Since you asked me specifically to take a look, my feedback is really that I won't take responsibility for targeting pack changes and we're going to be blocked on finding whoever is qualified (I don't know who), unless the native AOT change is separated out.
@jkotas
jkotas
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
I know that it is tempting to make multiple somewhat unrelated change in the same PR. I have done it myself many times, and sometimes had to revert it or split into multiple PRs to make review and approval easier.
@am11 Please take @MichalStrehovsky comment only as a suggestion. If you prefer to keep both changes in one PR, that's ok - it is still manageable. We appreciate your contributions.
@LuckyXu-HFLuckyXu-HF
mentioned this pull request
Jun 11, 2024
 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 4, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovsky

+5 more reviewers

@tomeksowitomeksowitomeksowi approved these changes
@clamp03clamp03clamp03 approved these changes
@gbalykovgbalykovgbalykov approved these changes
@shushanhfshushanhfshushanhf approved these changes
@BajtazarBajtazarBajtazar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

arch-loongarch64arch-riscvRelated to the RISC-V architecturearea-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@am11@gbalykov@MichalStrehovsky@jkotas@tomeksowi@clamp03@shushanhf@Bajtazar

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Align LA64 targetpacks with RV64 - #102628
Closed
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Closed
Align LA64 targetpacks with RV64#102628
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Conversation
@am11
@am11am11
commented
May 23, 2024

Copy link


Copy Markdown

Member
Also consolidates <NativeAotSupported definitions.
@ghostghost
added
the 
area-ReadyToRun

label
May 23, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 23, 2024
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
459b546 to
dfa88cbCompareMay 23, 2024 21:36
@am11am11
added
the 
arch-riscv
Related to the RISC-V architecture
label
May 23, 2024
@am11
am11
marked this pull request as ready for review
May 24, 2024 00:12
shushanhf
shushanhf
approved these changes
May 24, 2024
View reviewed changes
@shushanhfshushanhf
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM for LoongArch64.
Thanks very much!
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Closed

MichalStrehovsky
MichalStrehovsky
reviewed
May 24, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

Comment threadsrc/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj
Outdated

@gbalykov
gbalykov
commented
May 24, 2024

Copy link


Copy Markdown

Member
cc @dotnet/samsung
gbalykov
gbalykov
approved these changes
May 24, 2024
View reviewed changes
@gbalykovgbalykov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good to me
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
32d98b7 to
8479358CompareMay 24, 2024 10:00

This was referenced May 24, 2024

Closed


Closed


Closed


Closed

@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
b40bca0 to
7eec988CompareMay 29, 2024 16:04
@am11
am11
commented
May 29, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, @filipnavara, with #102760 merged the build error is fixed here. This is now publishing linux-arm crossgen2 as an AOT app (instead of R2R), bringing it to the same arm64,x64 plan.
@am11
am11
requested review from
jkoritzinsky and jkotasMay 31, 2024 08:04
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why do we need '$(CrossBuild)' != 'true' in the condition? I would expect '$(TargetOS)' == '$(HostOS)' to be enough.
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This part of the condition was adapted from main 
runtime/src/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj

Lines 13 to 14
in
cb71b60
<!-- Publish crossgen2 as a single-file app on native-OS builds. Cross-OS NativeAOT compilation is not supported yet -->
 <NativeAotSupportedCondition="'$(CrossBuild)' == 'true' and '$(TargetOS)' != '$(HostOS)'">false</NativeAotSupported> 
e.g. x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
@jkotasjkotasJun 2, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
This is situation should be covered by '$(TargetOS)' != '$(HostOS)' condition. I do not understand why we also check for CrossBuild in the condition.
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
 <_NativeAotSupportedOS Condition="'$(TargetOS)' == 'windows' or '$(TargetOS)' == 'linux' or '$(TargetOS)' == 'osx' or '$(TargetOS)' == 'maccatalyst' or '$(TargetOS)' == 'iossimulator' or '$(TargetOS)' == 'ios' or '$(TargetOS)' == 'tvossimulator' or '$(TargetOS)' == 'tvos' or '$(TargetOS)' == 'freebsd'">true</_NativeAotSupportedOS>
 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/blob/main/src/native/managed/compile-native.proj#L18-L28 is another place that checks for whether it is possible to use native AOT to produce binaries compiler by the repo. Can we use this property there as well?
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I am not sure whether NativeAotCanExecuteOnHost is the right name for this property. We always have ilc that can execute on host for all host OSes. The problem is that we do not always have a working host/target build environment.
Would something like UseNativeAotForComponents be a better name?
am11
am11
commented
Jun 2, 2024
View reviewed changes
Comment threadsrc/native/managed/compile-native.proj

 <!-- some special kinds of runtime builds need extra NativeAOT flags -->
 <PropertyGroup>
 <SysRoot Condition="'$(CrossBuild)' == 'true' and '$(HostOS)' != 'windows'">$(ROOTFS_DIR)</SysRoot>
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

These SysRoot setup can also be brought to the same plan as ILCompiler.csproj and crossgen2.csproj (better deduplicate all three using init-compiler.sh thingy). I have a feeling it will take some digging which is why @lambdageek chose this approach, but worth a try (in a follow-up PR). 🙂
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 2, 2024

Closed

@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
am11 requested review from jkoritzinsky, jkotas and MichalStrehovsky 3 days ago
I think it would help reviewing if you could split off the native AOT change to a separate PR. I don't know about target packs or RV64 and cannot review or sign off on that.
I (and probably others) often skim commit histories and PRs to see what's going on and skip stuff with titles that are not related to my areas - a PR named "Align LA64 targetpacks" that spends most lines of the diff on something unrelated breaks such workflows.
@am11am11
closed this
Jun 3, 2024
@am11
am11
commented
Jun 3, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
Sorry, it was not clear who would be responsible for getting this merged since it touched multiple things owned/worked on by different teams. I often comment on PRs without an intention of taking the full responsibility of merging it - whoever merged it is the one who's ultimately responsible even if it was authored by you. It was not clear to me who could merge a PR in this shape (I guess Jan often gets to volunteer himself for these, but I don't like dumping all work on Jan). Since you asked me specifically to take a look, my feedback is really that I won't take responsibility for targeting pack changes and we're going to be blocked on finding whoever is qualified (I don't know who), unless the native AOT change is separated out.
@jkotas
jkotas
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
I know that it is tempting to make multiple somewhat unrelated change in the same PR. I have done it myself many times, and sometimes had to revert it or split into multiple PRs to make review and approval easier.
@am11 Please take @MichalStrehovsky comment only as a suggestion. If you prefer to keep both changes in one PR, that's ok - it is still manageable. We appreciate your contributions.
@LuckyXu-HFLuckyXu-HF
mentioned this pull request
Jun 11, 2024
 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 4, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovsky

+5 more reviewers

@tomeksowitomeksowitomeksowi approved these changes
@clamp03clamp03clamp03 approved these changes
@gbalykovgbalykovgbalykov approved these changes
@shushanhfshushanhfshushanhf approved these changes
@BajtazarBajtazarBajtazar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

arch-loongarch64arch-riscvRelated to the RISC-V architecturearea-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@am11@gbalykov@MichalStrehovsky@jkotas@tomeksowi@clamp03@shushanhf@Bajtazar

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Align LA64 targetpacks with RV64 - #102628
Closed
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Closed
Align LA64 targetpacks with RV64#102628
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Conversation
@am11
@am11am11
commented
May 23, 2024

Copy link


Copy Markdown

Member
Also consolidates <NativeAotSupported definitions.
@ghostghost
added
the 
area-ReadyToRun

label
May 23, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 23, 2024
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
459b546 to
dfa88cbCompareMay 23, 2024 21:36
@am11am11
added
the 
arch-riscv
Related to the RISC-V architecture
label
May 23, 2024
@am11
am11
marked this pull request as ready for review
May 24, 2024 00:12
shushanhf
shushanhf
approved these changes
May 24, 2024
View reviewed changes
@shushanhfshushanhf
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM for LoongArch64.
Thanks very much!
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Closed

MichalStrehovsky
MichalStrehovsky
reviewed
May 24, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

Comment threadsrc/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj
Outdated

@gbalykov
gbalykov
commented
May 24, 2024

Copy link


Copy Markdown

Member
cc @dotnet/samsung
gbalykov
gbalykov
approved these changes
May 24, 2024
View reviewed changes
@gbalykovgbalykov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good to me
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
32d98b7 to
8479358CompareMay 24, 2024 10:00

This was referenced May 24, 2024

Closed


Closed


Closed


Closed

@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
b40bca0 to
7eec988CompareMay 29, 2024 16:04
@am11
am11
commented
May 29, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, @filipnavara, with #102760 merged the build error is fixed here. This is now publishing linux-arm crossgen2 as an AOT app (instead of R2R), bringing it to the same arm64,x64 plan.
@am11
am11
requested review from
jkoritzinsky and jkotasMay 31, 2024 08:04
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why do we need '$(CrossBuild)' != 'true' in the condition? I would expect '$(TargetOS)' == '$(HostOS)' to be enough.
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This part of the condition was adapted from main 
runtime/src/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj

Lines 13 to 14
in
cb71b60
<!-- Publish crossgen2 as a single-file app on native-OS builds. Cross-OS NativeAOT compilation is not supported yet -->
 <NativeAotSupportedCondition="'$(CrossBuild)' == 'true' and '$(TargetOS)' != '$(HostOS)'">false</NativeAotSupported> 
e.g. x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
@jkotasjkotasJun 2, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
This is situation should be covered by '$(TargetOS)' != '$(HostOS)' condition. I do not understand why we also check for CrossBuild in the condition.
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
 <_NativeAotSupportedOS Condition="'$(TargetOS)' == 'windows' or '$(TargetOS)' == 'linux' or '$(TargetOS)' == 'osx' or '$(TargetOS)' == 'maccatalyst' or '$(TargetOS)' == 'iossimulator' or '$(TargetOS)' == 'ios' or '$(TargetOS)' == 'tvossimulator' or '$(TargetOS)' == 'tvos' or '$(TargetOS)' == 'freebsd'">true</_NativeAotSupportedOS>
 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/blob/main/src/native/managed/compile-native.proj#L18-L28 is another place that checks for whether it is possible to use native AOT to produce binaries compiler by the repo. Can we use this property there as well?
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I am not sure whether NativeAotCanExecuteOnHost is the right name for this property. We always have ilc that can execute on host for all host OSes. The problem is that we do not always have a working host/target build environment.
Would something like UseNativeAotForComponents be a better name?
am11
am11
commented
Jun 2, 2024
View reviewed changes
Comment threadsrc/native/managed/compile-native.proj

 <!-- some special kinds of runtime builds need extra NativeAOT flags -->
 <PropertyGroup>
 <SysRoot Condition="'$(CrossBuild)' == 'true' and '$(HostOS)' != 'windows'">$(ROOTFS_DIR)</SysRoot>
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

These SysRoot setup can also be brought to the same plan as ILCompiler.csproj and crossgen2.csproj (better deduplicate all three using init-compiler.sh thingy). I have a feeling it will take some digging which is why @lambdageek chose this approach, but worth a try (in a follow-up PR). 🙂
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 2, 2024

Closed

@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
am11 requested review from jkoritzinsky, jkotas and MichalStrehovsky 3 days ago
I think it would help reviewing if you could split off the native AOT change to a separate PR. I don't know about target packs or RV64 and cannot review or sign off on that.
I (and probably others) often skim commit histories and PRs to see what's going on and skip stuff with titles that are not related to my areas - a PR named "Align LA64 targetpacks" that spends most lines of the diff on something unrelated breaks such workflows.
@am11am11
closed this
Jun 3, 2024
@am11
am11
commented
Jun 3, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
Sorry, it was not clear who would be responsible for getting this merged since it touched multiple things owned/worked on by different teams. I often comment on PRs without an intention of taking the full responsibility of merging it - whoever merged it is the one who's ultimately responsible even if it was authored by you. It was not clear to me who could merge a PR in this shape (I guess Jan often gets to volunteer himself for these, but I don't like dumping all work on Jan). Since you asked me specifically to take a look, my feedback is really that I won't take responsibility for targeting pack changes and we're going to be blocked on finding whoever is qualified (I don't know who), unless the native AOT change is separated out.
@jkotas
jkotas
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
I know that it is tempting to make multiple somewhat unrelated change in the same PR. I have done it myself many times, and sometimes had to revert it or split into multiple PRs to make review and approval easier.
@am11 Please take @MichalStrehovsky comment only as a suggestion. If you prefer to keep both changes in one PR, that's ok - it is still manageable. We appreciate your contributions.
@LuckyXu-HFLuckyXu-HF
mentioned this pull request
Jun 11, 2024
 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 4, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovsky

+5 more reviewers

@tomeksowitomeksowitomeksowi approved these changes
@clamp03clamp03clamp03 approved these changes
@gbalykovgbalykovgbalykov approved these changes
@shushanhfshushanhfshushanhf approved these changes
@BajtazarBajtazarBajtazar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

arch-loongarch64arch-riscvRelated to the RISC-V architecturearea-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@am11@gbalykov@MichalStrehovsky@jkotas@tomeksowi@clamp03@shushanhf@Bajtazar

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Align LA64 targetpacks with RV64 - #102628
Closed
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Closed
Align LA64 targetpacks with RV64#102628
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Conversation
@am11
@am11am11
commented
May 23, 2024

Copy link


Copy Markdown

Member
Also consolidates <NativeAotSupported definitions.
@ghostghost
added
the 
area-ReadyToRun

label
May 23, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 23, 2024
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
459b546 to
dfa88cbCompareMay 23, 2024 21:36
@am11am11
added
the 
arch-riscv
Related to the RISC-V architecture
label
May 23, 2024
@am11
am11
marked this pull request as ready for review
May 24, 2024 00:12
shushanhf
shushanhf
approved these changes
May 24, 2024
View reviewed changes
@shushanhfshushanhf
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM for LoongArch64.
Thanks very much!
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Closed

MichalStrehovsky
MichalStrehovsky
reviewed
May 24, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

Comment threadsrc/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj
Outdated

@gbalykov
gbalykov
commented
May 24, 2024

Copy link


Copy Markdown

Member
cc @dotnet/samsung
gbalykov
gbalykov
approved these changes
May 24, 2024
View reviewed changes
@gbalykovgbalykov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good to me
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
32d98b7 to
8479358CompareMay 24, 2024 10:00

This was referenced May 24, 2024

Closed


Closed


Closed


Closed

@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
b40bca0 to
7eec988CompareMay 29, 2024 16:04
@am11
am11
commented
May 29, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, @filipnavara, with #102760 merged the build error is fixed here. This is now publishing linux-arm crossgen2 as an AOT app (instead of R2R), bringing it to the same arm64,x64 plan.
@am11
am11
requested review from
jkoritzinsky and jkotasMay 31, 2024 08:04
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why do we need '$(CrossBuild)' != 'true' in the condition? I would expect '$(TargetOS)' == '$(HostOS)' to be enough.
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This part of the condition was adapted from main 
runtime/src/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj

Lines 13 to 14
in
cb71b60
<!-- Publish crossgen2 as a single-file app on native-OS builds. Cross-OS NativeAOT compilation is not supported yet -->
 <NativeAotSupportedCondition="'$(CrossBuild)' == 'true' and '$(TargetOS)' != '$(HostOS)'">false</NativeAotSupported> 
e.g. x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
@jkotasjkotasJun 2, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
This is situation should be covered by '$(TargetOS)' != '$(HostOS)' condition. I do not understand why we also check for CrossBuild in the condition.
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
 <_NativeAotSupportedOS Condition="'$(TargetOS)' == 'windows' or '$(TargetOS)' == 'linux' or '$(TargetOS)' == 'osx' or '$(TargetOS)' == 'maccatalyst' or '$(TargetOS)' == 'iossimulator' or '$(TargetOS)' == 'ios' or '$(TargetOS)' == 'tvossimulator' or '$(TargetOS)' == 'tvos' or '$(TargetOS)' == 'freebsd'">true</_NativeAotSupportedOS>
 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/blob/main/src/native/managed/compile-native.proj#L18-L28 is another place that checks for whether it is possible to use native AOT to produce binaries compiler by the repo. Can we use this property there as well?
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I am not sure whether NativeAotCanExecuteOnHost is the right name for this property. We always have ilc that can execute on host for all host OSes. The problem is that we do not always have a working host/target build environment.
Would something like UseNativeAotForComponents be a better name?
am11
am11
commented
Jun 2, 2024
View reviewed changes
Comment threadsrc/native/managed/compile-native.proj

 <!-- some special kinds of runtime builds need extra NativeAOT flags -->
 <PropertyGroup>
 <SysRoot Condition="'$(CrossBuild)' == 'true' and '$(HostOS)' != 'windows'">$(ROOTFS_DIR)</SysRoot>
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

These SysRoot setup can also be brought to the same plan as ILCompiler.csproj and crossgen2.csproj (better deduplicate all three using init-compiler.sh thingy). I have a feeling it will take some digging which is why @lambdageek chose this approach, but worth a try (in a follow-up PR). 🙂
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 2, 2024

Closed

@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
am11 requested review from jkoritzinsky, jkotas and MichalStrehovsky 3 days ago
I think it would help reviewing if you could split off the native AOT change to a separate PR. I don't know about target packs or RV64 and cannot review or sign off on that.
I (and probably others) often skim commit histories and PRs to see what's going on and skip stuff with titles that are not related to my areas - a PR named "Align LA64 targetpacks" that spends most lines of the diff on something unrelated breaks such workflows.
@am11am11
closed this
Jun 3, 2024
@am11
am11
commented
Jun 3, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
Sorry, it was not clear who would be responsible for getting this merged since it touched multiple things owned/worked on by different teams. I often comment on PRs without an intention of taking the full responsibility of merging it - whoever merged it is the one who's ultimately responsible even if it was authored by you. It was not clear to me who could merge a PR in this shape (I guess Jan often gets to volunteer himself for these, but I don't like dumping all work on Jan). Since you asked me specifically to take a look, my feedback is really that I won't take responsibility for targeting pack changes and we're going to be blocked on finding whoever is qualified (I don't know who), unless the native AOT change is separated out.
@jkotas
jkotas
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
I know that it is tempting to make multiple somewhat unrelated change in the same PR. I have done it myself many times, and sometimes had to revert it or split into multiple PRs to make review and approval easier.
@am11 Please take @MichalStrehovsky comment only as a suggestion. If you prefer to keep both changes in one PR, that's ok - it is still manageable. We appreciate your contributions.
@LuckyXu-HFLuckyXu-HF
mentioned this pull request
Jun 11, 2024
 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 4, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovsky

+5 more reviewers

@tomeksowitomeksowitomeksowi approved these changes
@clamp03clamp03clamp03 approved these changes
@gbalykovgbalykovgbalykov approved these changes
@shushanhfshushanhfshushanhf approved these changes
@BajtazarBajtazarBajtazar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

arch-loongarch64arch-riscvRelated to the RISC-V architecturearea-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@am11@gbalykov@MichalStrehovsky@jkotas@tomeksowi@clamp03@shushanhf@Bajtazar

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Align LA64 targetpacks with RV64 - #102628
Closed
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Closed
Align LA64 targetpacks with RV64#102628
am11 wants to merge 13 commits into
dotnet:mainfrom 
am11:feature/crossgen2/LA64
Conversation
@am11
@am11am11
commented
May 23, 2024

Copy link


Copy Markdown

Member
Also consolidates <NativeAotSupported definitions.
@ghostghost
added
the 
area-ReadyToRun

label
May 23, 2024
@dotnet-policy-servicedotnet-policy-serviceBot
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
May 23, 2024
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
459b546 to
dfa88cbCompareMay 23, 2024 21:36
@am11am11
added
the 
arch-riscv
Related to the RISC-V architecture
label
May 23, 2024
@am11
am11
marked this pull request as ready for review
May 24, 2024 00:12
shushanhf
shushanhf
approved these changes
May 24, 2024
View reviewed changes
@shushanhfshushanhf
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM for LoongArch64.
Thanks very much!
@build-analysisbuild-analysisBot
mentioned this pull request
May 24, 2024

Closed

MichalStrehovsky
MichalStrehovsky
reviewed
May 24, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

Comment threadsrc/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj
Outdated

@gbalykov
gbalykov
commented
May 24, 2024

Copy link


Copy Markdown

Member
cc @dotnet/samsung
gbalykov
gbalykov
approved these changes
May 24, 2024
View reviewed changes
@gbalykovgbalykov
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Looks good to me
@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
32d98b7 to
8479358CompareMay 24, 2024 10:00

This was referenced May 24, 2024

Closed


Closed


Closed


Closed

@am11
am11force-pushed
the
feature/crossgen2/LA64
branch
from
b40bca0 to
7eec988CompareMay 29, 2024 16:04
@am11
am11
commented
May 29, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, @filipnavara, with #102760 merged the build error is fixed here. This is now publishing linux-arm crossgen2 as an AOT app (instead of R2R), bringing it to the same arm64,x64 plan.
@am11
am11
requested review from
jkoritzinsky and jkotasMay 31, 2024 08:04
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Why do we need '$(CrossBuild)' != 'true' in the condition? I would expect '$(TargetOS)' == '$(HostOS)' to be enough.
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This part of the condition was adapted from main 
runtime/src/coreclr/tools/aot/crossgen2/crossgen2_publish.csproj

Lines 13 to 14
in
cb71b60
<!-- Publish crossgen2 as a single-file app on native-OS builds. Cross-OS NativeAOT compilation is not supported yet -->
 <NativeAotSupportedCondition="'$(CrossBuild)' == 'true' and '$(TargetOS)' != '$(HostOS)'">false</NativeAotSupported> 
e.g. x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
@jkotasjkotasJun 2, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

x64 binary running on arm64 OS is supported (as long as the host and target OS were same).
This is situation should be covered by '$(TargetOS)' != '$(HostOS)' condition. I do not understand why we also check for CrossBuild in the condition.
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
 <_NativeAotSupportedOS Condition="'$(TargetOS)' == 'windows' or '$(TargetOS)' == 'linux' or '$(TargetOS)' == 'osx' or '$(TargetOS)' == 'maccatalyst' or '$(TargetOS)' == 'iossimulator' or '$(TargetOS)' == 'ios' or '$(TargetOS)' == 'tvossimulator' or '$(TargetOS)' == 'tvos' or '$(TargetOS)' == 'freebsd'">true</_NativeAotSupportedOS>
 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

https://github.com/dotnet/runtime/blob/main/src/native/managed/compile-native.proj#L18-L28 is another place that checks for whether it is possible to use native AOT to produce binaries compiler by the repo. Can we use this property there as well?
jkotas
jkotas
reviewed
Jun 2, 2024
View reviewed changes
Comment threadeng/Subsets.props
Outdated

 <_NativeAotSupportedArch Condition="'$(TargetArchitecture)' == 'x64' or '$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm' or ('$(TargetOS)' == 'windows' and '$(TargetArchitecture)' == 'x86')">true</_NativeAotSupportedArch>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and $(_NativeAotSupportedArch) == 'true'">true</NativeAotSupported>
 <NativeAotSupported Condition="'$(_NativeAotSupportedOS)' == 'true' and '$(_NativeAotSupportedArch)' == 'true'">true</NativeAotSupported>
 <NativeAotCanExecuteOnHost Condition="'$(NativeAotSupported)' == 'true' and ('$(CrossBuild)' != 'true' or '$(TargetOS)' == '$(HostOS)')">true</NativeAotCanExecuteOnHost>
@jkotasjkotasJun 2, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I am not sure whether NativeAotCanExecuteOnHost is the right name for this property. We always have ilc that can execute on host for all host OSes. The problem is that we do not always have a working host/target build environment.
Would something like UseNativeAotForComponents be a better name?
am11
am11
commented
Jun 2, 2024
View reviewed changes
Comment threadsrc/native/managed/compile-native.proj

 <!-- some special kinds of runtime builds need extra NativeAOT flags -->
 <PropertyGroup>
 <SysRoot Condition="'$(CrossBuild)' == 'true' and '$(HostOS)' != 'windows'">$(ROOTFS_DIR)</SysRoot>
@am11am11Jun 2, 2024

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

These SysRoot setup can also be brought to the same plan as ILCompiler.csproj and crossgen2.csproj (better deduplicate all three using init-compiler.sh thingy). I have a feeling it will take some digging which is why @lambdageek chose this approach, but worth a try (in a follow-up PR). 🙂
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 2, 2024

Closed

@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
am11 requested review from jkoritzinsky, jkotas and MichalStrehovsky 3 days ago
I think it would help reviewing if you could split off the native AOT change to a separate PR. I don't know about target packs or RV64 and cannot review or sign off on that.
I (and probably others) often skim commit histories and PRs to see what's going on and skip stuff with titles that are not related to my areas - a PR named "Align LA64 targetpacks" that spends most lines of the diff on something unrelated breaks such workflows.
@am11am11
closed this
Jun 3, 2024
@am11
am11
commented
Jun 3, 2024

Copy link


Copy Markdown

MemberAuthor
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
@MichalStrehovsky
MichalStrehovsky
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
@MichalStrehovsky, the PR description has the line and if PR name was the issue, you or @jkotas could have told a bit earlier without going through the cycles of feedback and it would have saved both some time.
Sorry, it was not clear who would be responsible for getting this merged since it touched multiple things owned/worked on by different teams. I often comment on PRs without an intention of taking the full responsibility of merging it - whoever merged it is the one who's ultimately responsible even if it was authored by you. It was not clear to me who could merge a PR in this shape (I guess Jan often gets to volunteer himself for these, but I don't like dumping all work on Jan). Since you asked me specifically to take a look, my feedback is really that I won't take responsibility for targeting pack changes and we're going to be blocked on finding whoever is qualified (I don't know who), unless the native AOT change is separated out.
@jkotas
jkotas
commented
Jun 3, 2024

Copy link


Copy Markdown

Member
I know that it is tempting to make multiple somewhat unrelated change in the same PR. I have done it myself many times, and sometimes had to revert it or split into multiple PRs to make review and approval easier.
@am11 Please take @MichalStrehovsky comment only as a suggestion. If you prefer to keep both changes in one PR, that's ok - it is still manageable. We appreciate your contributions.
@LuckyXu-HFLuckyXu-HF
mentioned this pull request
Jun 11, 2024
 Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Jul 4, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas left review comments
@jkoritzinskyjkoritzinskyAwaiting requested review from jkoritzinsky
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovsky

+5 more reviewers

@tomeksowitomeksowitomeksowi approved these changes
@clamp03clamp03clamp03 approved these changes
@gbalykovgbalykovgbalykov approved these changes
@shushanhfshushanhfshushanhf approved these changes
@BajtazarBajtazarBajtazar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

arch-loongarch64arch-riscvRelated to the RISC-V architecturearea-ReadyToRuncommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@am11@gbalykov@MichalStrehovsky@jkotas@tomeksowi@clamp03@shushanhf@Bajtazar