-
Notifications
You must be signed in to change notification settings - Fork 349
board: intel_adsp_ace15_mtpm xtensa toolchain upgrade and enable xt-clang #7027
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
board: intel_adsp_ace15_mtpm xtensa toolchain upgrade and enable xt-clang #7027
Conversation
a2a8ee7 to
8ab901c
Compare
|
I do not understand why zephyr builds CI is failing. It uses Zephyr SDK so no change done. It stops with weird error during west update step.... |
|
That's probably because we use |
scripts/xtensa-build-zephyr.py
Outdated
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.
Can you please provide a default value here?
| "'xcc' or 'xcc-clang'") | |
| if not os.getenv("ZEPHYR_TOOLCHAIN_VARIANT"): | |
| os.environ["ZEPHYR_TOOLCHAIN_VARIANT"] = "xcc-clang" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think default value is a good idea here especially one set to xt-clang. Script end-user if not set this variable could be trying to compile TGL with xt-clang what is possible but what we do not support (well at least I had not checked it for compiler errors/warnings). Also TGL does not have xcc-clang toolchain enabled in Zephyr board yaml file. I'm not sure what would happen.
Is this value needed by some CI systems?
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.
Is this value needed by some CI systems?
If it's optional then why fail when it's missing?
if not set this variable could be trying to compile TGL with xt-clang what is possible but what we do not support [...]
It's OK to fail in unsupported cases. It's not OK to require a new, "evil" environment variable for the default case.
I'm not sure what would happen.
If the error message for some unsupported case is too cryptic, the few people who want/need that case can always submit a better error message.
Also TGL does not have xcc-clang toolchain enabled in Zephyr board yaml file.
BTW the platform_list has always provided platform-specific default values. In fact platform_list is probably the main value of this script: platform-specific default values that allow building multiple platforms at once without any painful configuration tricks. It will be a real regression to lose that.
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.
Hmm, yeah you are right.
Maybe approach with expanding platform_list values would be much better solution (we just did not have to distinguish between Xtensa compilers before). I'll just add new field to platform_list that would provide info that for TGL default toolchain is xt-xcc and for MTL is xt-clang. That would make much more sense.
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.
@marc-hb does this looks better 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.
It does look better thanks!
It fails to build MTL in https://sof-ci.01.org/sofpr/PR7027/build3671/build/mtl_zph_ipc4_error.txt, looks like we need to upgrade our XCC toolchain. @aiChaoSONG , @keqiaozhang , @fredoh9 ?
8ab901c to
00deaf7
Compare
marc-hb
left a comment
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.
Intel Meteorlake release toolchain is Xtensa RI-2021.7 version. New toolchain does not support xt-xcc driver anymore so we are forced to switch to new xt-clang driver.
Wait, this sounds like a "Flag Day" change? I mean: there is no "transition" toolchain that supports both xt-xcc and xt-clang, it's either one or the other? That sounds bad, it sounds like only the "before" version of this script will be compatible with older toolchains and only the "after" version of this script will be compatible with newer toolchains? OK now maybe I understand why you were asking for a new input...
Assuming I'm understanding all this correctly, I think we need both a default value for convenience and a new input parameter so users can use git across the "flag day" without having to switch toolchain at the same time. See inline suggestions.
scripts/xtensa-build-zephyr.py
Outdated
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.
It does look better thanks!
It fails to build MTL in https://sof-ci.01.org/sofpr/PR7027/build3671/build/mtl_zph_ipc4_error.txt, looks like we need to upgrade our XCC toolchain. @aiChaoSONG , @keqiaozhang , @fredoh9 ?
|
The massive regression of most tests in https://sof-ci.01.org/sofpr/PR7027/build3671/devicetest/index.html is likely caused by unrelated Zephyr code indirectly included by the Probably Zephyr regression #6965 (comment) |
|
I installed new toolchains, RI-2021.7 and RI-2022.10 (both builds and tools). Looks like MTL use RI-2021.7 builds and tools. I was able to build MTL fw with main. Then pull this PR and try again. But build failed. |
|
Looks like the tool install didn't finish properly. Re-installed again. Now I'm able to build MTL fw. When I had errors, After the fixed the registry problem, |
That's why all our build scripts define XTENSA_SYSTEM, so you don't have to and you don't have to "fix the registry". |
|
SOFCI TEST |
1 similar comment
|
SOFCI TEST |
|
All build machines have new toolchain, RI-2021.7 and ace10_LX7HiFi4 |
|
By the way, are you planning to update with RI-2022.10 also? If we have a plan to upgrade to RI-2022.10 sooner or later let me know also. So that we can prepare for that also. |
lgirdwood
left a comment
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.
@aborisovich any ETA for non draft ?
Hi, yes I've asked our process team to install RI-2022.10 straight away so I guess we will just skip 2021 toolchain. So please prepare for 2022.10 on sof CI side @fredoh9 . |
I can make it normal Open PR but it has dependency to Zephyr PR with xt-clang introduction and "-no-pie" unsupported flag enforced by Zephyr toolchain. We will discuss those hanging PRs tomorrow with Zephyr team. I think code freeze is ended and those should be merged soon. |
|
zephyrproject-rtos/zephyr#54836 would probably have a higher chance of getting merged sooner. |
00deaf7 to
f46c2d2
Compare
@aborisovich could you fast-track the C code changes in a new, separate PR? It's easy enough for users to locally hack one-line in the python script and ignore the Once the C code is fixed it's also easier for Zephyr people like @dcpleung to give SOF a quick try. |
PRs like those give me joy. Finally those names had been fixed, that's what I've been talking about @marc-hb . |
Sure, I'll create new PR which we can instantly merge without any dependencies. |
|
Created new PR #7096 just for fixing C warnings. |
f46c2d2 to
757a8c7
Compare
|
Ok, got info that our Intel Internal CI has new toolchains set up. |
|
Can one of the admins verify this patch? |
|
@jxstelter can you please confirm this switch to xt-clang and C++17 standard? |
scripts/xtensa-build-zephyr.py
Outdated
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.
verbose by default?
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.
Please have a look at commits.
There is one commit 9ae133d signed [DO NOT MERGE] that will be dropped.
It was added temporary to validate CI logs on Intel Internal CI and SOF CI.
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.
Approved except of course the DO NOT MERGE commit. Demoting to draft to avoid accidental merges, it happened :-) It's just one click both ways.
I'm not happy with the new C++17 requirement because it's not clear why it's needed: the author claims the requirement is still C++11 in
zephyrproject-rtos/zephyr#53405 (comment)
But it's just a one-line, super easy to change later.
added requirement to SOF project to use C++17 standard. This was caused by changes in Zephyr lib: os: PoT utilities, hash function, hash map (hash table) zephyrproject-rtos/zephyr#53405 that require C++14 standard.
@aborisovich can you please file a bug for this with (hopefully easy and open-source) reproduction steps?
SOF CI device tests failed check-suspend-resume-with-capture.sh on ADLP_RVP_SDW_IPC4ZPH and TGLU_RVP_NOCODEC_IPC4ZPH but those seems unrelated as TGL had been build using existing xt-xcc toolchain.
We have had suspend/resume failures since forever.
Always copy the URL because it gets lost after force push:
https://sof-ci.01.org/sofpr/PR7027/build4128/devicetest/index.html
scripts/xtensa-build-zephyr.py
Outdated
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.
Note for other reviewers: this is part of the temporary, DO NOT MERGE commit. Do not review. It looked "too good" so I wasted a bit of time reviewing it, next time please add some uglier comments :-)
I've submitted the bug zephyrproject-rtos/zephyr#55204 explaining the situation and containing the proof that Zephyr code requires C++14 after the PR we mentioned earlier. |
|
Still waiting for the MTL full scope test results. We will have a results tomorrow. If no regression proven we can merge. |
Intel Meteorlake release toolchain is Xtensa RI-2022.10 version. New toolchain does not support xt-xcc driver anymore so we are forced to switch to new xt-clang driver. To distinguish between default driver for the platform built, (Xtensa GCC or Clang) added new field to script platform list containing a toolchain name "xcc" or "xcc-clang" expected by Zephyr project. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>
Recent changes to Zephyr header files written in C++ use templates and features provided by the C++14 standard. Meteorlake board that is built with Zephyr has some C++ code that uses those headers. We upgrade straight to C++17 since it is newest standard currently supported by xt-clang toolchain. This change does not affect any other boards since the do not have any C++ code as the time of writing. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>
Includes: - rename of xcc-clang compiler to xt-clang - updates in C++ headers in zephyr/sys/util.h that require C++14 standard now. Signed-off-by: Andrey Borisovich <andrey.borisovich@intel.com>
9ae133d to
780c2ab
Compare
|
Results from validation team proved no regression on MTL full scope. Removed [DO NOT MERGE] commit, Intel Internal CI had switched to use xt-clang. Current force push will use new toolchain. Waiting for the results and if everything good we can merge @lgirdwood . |
|
Ok all green. |
Release toolchain for Intel Meteorlake board is Xtensa RI-2022.10.
Upgraded toolchain to final version. New Xtensa toolchains does not support obsolete xt-xcc driver anymore,
so we need to switch to new xt-clang driver.
In this PR:
xtensa-build-system.pyto build mtl platform with new Xtensa toolchain version RI-2022.10 and core ace10_LX7HiFi4_2022_10xtensa-build-system.pyto use new compiler xt-clang as the xt-xcc is not available anymorein new Xtensa toolchains.
MTL has cpp code that uses those headers but we can move straight away to C++17 as the newest standard supported by xt-clang in this toolchain.
Merging this PR will require synchronization with the Intel Internal CI team so they can change environmental variables on their side that would select new toolchain as default in production.
Dependencies: