-
Notifications
You must be signed in to change notification settings - Fork 538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[One .NET] support $(EnableLLVM) with AOT #6157
Conversation
01b470a
to
81eb347
Compare
7278f00
to
d731113
Compare
@@ -150,8 +158,6 @@ public void BuildBasicApplicationReleaseProfiledAotWithoutDefaultProfile () | |||
[Category ("DotNetIgnore")] // Not currently working, see: https://github.com/dotnet/runtime/issues/56163 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to remove these now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests that mention dotnet/runtime#56163 don't work yet. I think we need changes in dotnet/runtime to fix those.
Folder names like BuildAotApplicationAndBundle AndÜmläüts_arm64-v8a_False_True
fail, and I saw the <MonoAOTCompiler/>
task output characters that looked like the encoding was messed up.
@@ -230,8 +236,6 @@ public void BuildBasicApplicationReleaseProfiledAotWithoutDefaultProfile () | |||
[Category ("DotNetIgnore")] // Not currently working, see: https://github.com/dotnet/runtime/issues/56163 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something in the latest changes broke legacy:
[aot-compiler stderr] '"opt"' is not recognized as an internal or external command, (TaskId:180)
I thought it was green, but then I changed some other things...
Fixed some issues that allows `$(EnableLLVM)` to work: * `$(_LLVMPath)` needs to point to Mono's AOT pack that contains `opt` and `llc`. * We need to fill out `ld-name` and `ld-flags`. * `ld-flags` needs some special workarounds in order for them to be passed in correctly on .NET 6. * Use space as a delimiter * Escape spaces in paths * Added `temp-path` parameter. This prevents the warning on Windows: 'rm' is not recognized as an internal or external command I enabled tests around .NET 6 and LLVM. ~~ Results ~~ All tests: 1. Were running on a [Google Pixel 5][0], and 2. Enabled two architectures, arm64 and x86, and 3. **AOT time** was average of 10 runs with `-c Release -p:RunAOTCompilation=true`, with the `Activity: Displayed` time 4. **AOT+LLVM time** was average of 10 runs with `-c Release -p:RunAOTCompilation=true -p:EnableLLVM=true` with the `Activity: Displayed` time. | Test | AOT time | AOT+LLVM time | AOT apk size | AOT+LLVM apk size | | ------------------- | ------------: | ------------: | ------------: | -----------------: | | [HelloAndroid][1] | 00:00:00.238 | 00:00:00.226 | 12,073,931 | 12,602,315 | | [HelloMaui][2] | 00:00:00.624 | 00:00:00.596 | 43,167,801 | 47,546,425 | [0]: https://store.google.com/us/product/pixel_5_specs?hl=en-US [1]: https://github.com/dotnet/maui-samples/tree/714460431541f40570e91225e8ba4bc1fe08025f/HelloAndroid [2]: https://github.com/dotnet/maui-samples/tree/714460431541f40570e91225e8ba4bc1fe08025f/HelloMaui
d731113
to
9ae21df
Compare
// NOTE: ordering seems to matter on Windows | ||
aotOptions.Insert (0, $"outfile={outputFile}"); | ||
aotOptions.Insert (0, $"llvm-path={SdkBinDirectory}"); | ||
aotOptions.Insert (0, $"temp-path={tempDir}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR build is green (just seems like Github status didn't update).
The most recent thing that broke was the ordering of these parameters. It seems like the AOT compiler wants llvm-path
before ld-name
or ld-flags
.
@jonathanpeppers I would feel more comfortable if we ran the entire test suite on this (if we are not already) rather than relying on the targeted tests which we normally do. Messing with aot path quoting etc is tricky and can break in unexpected places. |
@dellis1972 the test run should have every step, nothing skipped: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=5055201&view=results If a file changes in |
Fixed some issues that allows
$(EnableLLVM)
to work:$(_LLVMPath)
needs to point to Mono's AOT pack that containsopt
and
llc
.ld-name
andld-flags
.ld-flags
needs some special workarounds in order for them to bepassed in correctly on .NET 6.
temp-path
parameter. This prevents the warning on Windows:I enabled tests around .NET 6 and LLVM.
Results
All tests:
-c Release -p:RunAOTCompilation=true
, with theActivity: Displayed
time-c Release -p:RunAOTCompilation=true -p:EnableLLVM=true
with theActivity: Displayed
time.