This is an archive of the discontinued LLVM Phabricator instance.

Add build for Windows on Arm in packaging script
ClosedPublic

Authored by pbo-linaro on Jan 31 2023, 7:04 AM.

Diff Detail

Event Timeline

pbo-linaro created this revision.Jan 31 2023, 7:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
pbo-linaro requested review of this revision.Jan 31 2023, 7:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 7:04 AM

This script is used by Linaro to create LLVM Windows on Arm releases, and we would like to upstream this.

This looks good to me after fixing nits mentioned inline.

llvm/utils/release/build_llvm_release.bat
324

These flags are common to all stage1 builds and can be added to variable that be included to all stage1 builds.

333

These LLDB python related flags should be common for all Stage1 builds too. Should be added to a common variable.

pbo-linaro updated this revision to Diff 493916.Feb 1 2023, 5:26 AM

Factorized some flags.

pbo-linaro marked 2 inline comments as done.Feb 1 2023, 5:27 AM
pbo-linaro added inline comments.
llvm/utils/release/build_llvm_release.bat
324

stage0_bin_dir needs to be expanded for every architecture, so it has to be duplicated for all stage1.

333

Added them to common_flags for arm64 stage0 and stage1

hans added a comment.Feb 1 2023, 6:50 AM

Could the refactorings (set_environment and do_build_libxml) be done in separate patches? That would make it easier to review.

Side question: has anyone tried the script on llvm-16.0.0-rc1 yet? I was meaning to do it but didn't find the cycles yet.

llvm/utils/release/build_llvm_release.bat
144

Why is this being dropped?

303

We're building flang now?

330

..and mlir?

I wonder if we should try to keep LLVM_ENABLE_PROJECTS the same across targets, maybe there could be a flag to add flang to it if we need that.

378

This was probably my fault, but maybe add some newlines while here anyway.

pbo-linaro marked 2 inline comments as done.EditedFeb 1 2023, 7:01 AM

Thanks for your review!

Initially, I duplicated the LIBXML2 build, as you say, with the intent to factorize it later.
But finally, with @omjavaid , we decided to make both at once, so the result is cleaner.

It's a bit long to test (takes hours to build, and we did it many time), so if we could integrate both, that would be really nice...

For flags and list of targets to build, I'll let @omjavaid answer, as I followed his indications.

We built 17-init and 16.0.0-rc1 with this script, as shown here (arm64 only):
https://gitlab.com/Linaro/windowsonarm/packages/llvm-release/-/jobs/3694968725
We build it nightly as well, so we can have an updated llvm for our internal usage.

pbo-linaro marked an inline comment as done.Feb 1 2023, 7:03 AM
pbo-linaro added inline comments.
llvm/utils/release/build_llvm_release.bat
144

This adds a regression, as LLD_VERSION is not correctly detected, thus empty.
And it fails the cmake step, when a REGEX REPLACE is done on that empty string.

378

I'm not sure to understand what you want me to add exactly here?

hans added a comment.Feb 1 2023, 9:49 AM

Thanks for your review!

Initially, I duplicated the LIBXML2 build, as you say, with the intent to factorize it later.
But finally, with @omjavaid , we decided to make both at once, so the result is cleaner.

It's a bit long to test (takes hours to build, and we did it many time), so if we could integrate both, that would be really nice...

Okay.

For flags and list of targets to build, I'll let @omjavaid answer, as I followed his indications.

We built 17-init and 16.0.0-rc1 with this script, as shown here (arm64 only):
https://gitlab.com/Linaro/windowsonarm/packages/llvm-release/-/jobs/3694968725
We build it nightly as well, so we can have an updated llvm for our internal usage.

Great!

llvm/utils/release/build_llvm_release.bat
144

I think this line has been there for a long time though, so I don't understand how it could regress something. Maybe something else changed?
I don't think we should drop this, but try to fix the underlying problem.

378

I meant it would be nice to break this very long line into multiple lines (with ^ at the end, like we do with the set cmake_flags=... line for example).

pbo-linaro marked 3 inline comments as done.Feb 1 2023, 10:40 AM
pbo-linaro added inline comments.
llvm/utils/release/build_llvm_release.bat
144

You are right. After looking more in details, it happens only when PACKAGE_VERSION does not respect "[0-9]+.[0-9]+.*" regex.

Initially, I worked on that script using main branch, which resulted in -DPACKAGE_VERSION=main. 17-init presented the same issue, thus I assumed there had been a regression (and didn't try with 16.0.0-rc1, which works!).

As you can try yourself, this fails at cmake step.
cmake -GNinja -DLLVM_ENABLE_PROJECTS=lld -DCMAKE_BUILD_TYPE=Release -DPACKAGE_VERSION=main ./llvm
...

  • LLD version:

CMake Error at .../llvm-project/lld/CMakeLists.txt:137 (string):

string sub-command REGEX, mode REPLACE needs at least 6 arguments total to
command.

That said, for now, i'll just restore that line in the script, as we expect to build only "normal" releases with this script, matching the expected regex.

378

Yes sure. I forgot about this long line, as it's wrapped in phabricator.

pbo-linaro marked 2 inline comments as done.

Bring back PACKAGE_VERSION and split libxml2 cmake parameters on multiple lines.

pbo-linaro marked an inline comment as done.Feb 1 2023, 10:58 AM

@pbo-linaro I have replied to flang query and also going to issue a build using this script to verify it. Looks good to me. I ll post the result of my build tomorrow.

llvm/utils/release/build_llvm_release.bat
303

We have been releasing building flang + mlir for windows on arm since 14.0.0 release. I have tried it on x64 and it passes all ninja check-flang on x64. So I found it reasonable to include flang for all the windows build.

330

mlir + flang included to woa build since 14.0.0. IMO should be added to x64 as well.

hans added inline comments.Feb 2 2023, 9:19 AM
llvm/utils/release/build_llvm_release.bat
330

What does including mlir even mean since we build with -DLLVM_INSTALL_TOOLCHAIN_ONLY=ON? I thought it was a library?

As for flang, I'd suggest adding it behind a flag for now, which controls whether it gets added to ENABLE_PROJECTS centrally. You could default the flag to on for arm64 I suppose since it's been shipping there already.

omjavaid added inline comments.Feb 3 2023, 12:31 AM
llvm/utils/release/build_llvm_release.bat
330

Agreed. We only need clang at stage0 and for stage1 only arm64 can have mlir+flang.

Use same list of projects for arm64 than x86/x64, except for openmp which does
not build for arm64 at this time.

After discussion with @omjavaid, we decided to build same list of projects for arm64, than x64/x86.

The flang/mlir support could be added later with another patch.
Does this seem good to you @hans ?

Use same list of projects for arm64 than x86/x64, except for openmp which does
not build for arm64 at this time.

FYI, I think openmp should build for windows/arm64 with clang-cl with current git main (and on the 16.x branch); this was fixed in November/December. I primarily build and test it in mingw mode, but I've made fixes to make it work in clang-cl mode too.

Building openmp is a bit more tricky than other subprojects though, as it requires perl to be available at build time.

pbo-linaro added a comment.EditedFeb 3 2023, 1:03 AM

Thanks for the information @mstorsjo.
Let me try it on my machine, and I'll use the exact same projects list between all arch if it works 👍.

EDIT: works!

pbo-linaro updated this revision to Diff 494591.Feb 3 2023, 5:20 AM

Build same projects for all architecture.

OpenMP builds fine for arm64, as show in this build log:
https://gitlab.com/Linaro/windowsonarm/packages/llvm-release/-/jobs/3708866040

hans accepted this revision.Feb 3 2023, 9:48 AM

lgtm

After discussion with @omjavaid, we decided to build same list of projects for arm64, than x64/x86.

The flang/mlir support could be added later with another patch.
Does this seem good to you @hans ?

Sounds good to me.

llvm/utils/release/build_llvm_release.bat
308

What's the state of these tests on win/arm64? Should we enable some in a follow-up patch?

334

With all the other tests commented out, I'm surprised these are running.. do they pass? Should they be commented out too for now?

This revision is now accepted and ready to land.Feb 3 2023, 9:48 AM

Thanks for your approval! 👍

llvm/utils/release/build_llvm_release.bat
334

At the time of 15.0.0, there were some failures. We deactivated it as well to reduce build time.

But the plan is to enable them again in the future. First, we wanted to upstream that build script, to avoid keeping a fork on our side.

@hans As I don't have any commit access to LLVM, could you kindly push this patch? Thanks

This revision was automatically updated to reflect the committed changes.