Skip to content

Conversation

@aborisovich
Copy link
Contributor

@aborisovich aborisovich commented Feb 1, 2023

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:

  • modified xtensa-build-system.py to build mtl platform with new Xtensa toolchain version RI-2022.10 and core ace10_LX7HiFi4_2022_10
  • modified processes and xtensa-build-system.py to use new compiler xt-clang as the xt-xcc is not available anymore
    in new Xtensa toolchains.
  • 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.
    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.
  • please note: commit 9ae133d1abedd0ab8ebc8999954cf249648e84e7 will be dropped from this PR before merging. It contains temporary code to test new toolchain in Intel Internal CI & SOF CI and also provide debug logging.

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:

@aborisovich aborisovich force-pushed the mtl-xtensa-toolchain-upgrade branch from a2a8ee7 to 8ab901c Compare February 1, 2023 22:24
@aborisovich
Copy link
Contributor Author

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....

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 1, 2023

That's probably because we use west update --narrow to speed things up. Try either the full, 40 digit SHA1 or pull/54333/head or pull/54333/merge (without changing the remote)

Copy link
Collaborator

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?

Suggested change
"'xcc' or 'xcc-clang'")
if not os.getenv("ZEPHYR_TOOLCHAIN_VARIANT"):
os.environ["ZEPHYR_TOOLCHAIN_VARIANT"] = "xcc-clang"

Copy link
Contributor Author

@aborisovich aborisovich Feb 1, 2023

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

@aborisovich aborisovich Feb 2, 2023

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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 ?

@aborisovich aborisovich force-pushed the mtl-xtensa-toolchain-upgrade branch from 8ab901c to 00deaf7 Compare February 2, 2023 10:24
Copy link
Collaborator

@marc-hb marc-hb left a 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.

cc: @dcpleung, @aasthagr

Copy link
Collaborator

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 ?

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 2, 2023

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 west.yml update in this PR. Other, recent PRs are passing fine (examples https://sof-ci.sh.intel.com/#/result/planresultdetail/20432 and https://sof-ci.01.org/sofpr/PR7032/build3673/devicetest/index.html) and daily tests 20432 are fine too.

Probably Zephyr regression #6965 (comment)

@fredoh9
Copy link
Contributor

fredoh9 commented Feb 10, 2023

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.

:~/work/zwork/sof$ ./scripts/xtensa-build-zephyr.py mtl
Found west: /home/fredoh/.local/bin/west
Building platforms: mtl
In dir: /home/fredoh/work/zwork; running command: west topdir
In dir: /home/fredoh/work/zwork; running command: west config manifest.path
In dir: /home/fredoh/work/zwork; running command: west config manifest.file
West workspace: /home/fredoh/work/zwork
West manifest path: /home/fredoh/work/zwork/sof/west.yml
SOF_TOP=/home/fredoh/work/zwork/sof
west_top=/home/fredoh/work/zwork
Cleaning mtl from /home/fredoh/work/zwork/build-sof-staging
XTENSA_TOOLCHAIN_PATH=/home/fredoh/xcc/install/tools
TOOLCHAIN_VER=RI-2021.7-linux
XTENSA_SYSTEM=/home/fredoh/xcc/install/builds/RI-2021.7-linux/ace10_LX7HiFi4/config
In dir: /home/fredoh/work/zwork; running command: west build --build-dir build-mtl --board intel_adsp_ace15_mtpm /home/fredoh/work/zwork/sof/app --
-- west build: generating a build system
Loading Zephyr default modules (Zephyr base (cached)).
-- Application: /home/fredoh/work/zwork/sof/app
-- Cache files will be written to: /home/fredoh/.cache/zephyr
-- Zephyr version: 3.2.99 (/home/fredoh/work/zwork/zephyr)
-- Found west (found suitable version "0.14.0", minimum required is "0.7.1")
-- Board: intel_adsp_ace15_mtpm
-- Found toolchain: xcc-clang (/home/fredoh/xcc/install/tools)
CMake Error at /home/fredoh/work/zwork/zephyr/cmake/compiler/xcc/generic.cmake:25 (message):
  Executing the below command failed.  Are permissions set correctly?

  '/home/fredoh/xcc/install/tools/RI-2021.7-linux/XtensaTools/bin/xt-clang
  --version'

Call Stack (most recent call first):
  /home/fredoh/work/zwork/zephyr/cmake/compiler/xcc-clang/generic.cmake:3 (include)
  /home/fredoh/work/zwork/zephyr/cmake/modules/FindHostTools.cmake:107 (include)
  /home/fredoh/work/zwork/zephyr/cmake/modules/dts.cmake:8 (find_package)
  /home/fredoh/work/zwork/zephyr/cmake/modules/zephyr_default.cmake:108 (include)
  /home/fredoh/work/zwork/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:66 (include)
  /home/fredoh/work/zwork/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:97 (include_boilerplate)
  CMakeLists.txt:5 (find_package)
...

@fredoh9
Copy link
Contributor

fredoh9 commented Feb 10, 2023

Looks like the tool install didn't finish properly. Re-installed again. Now I'm able to build MTL fw.

When I had errors,

:~/work/zwork/sof$ /home/fredoh/xcc/install/tools/RI-2021.7-linux/XtensaTools/bin/xt-clang --version
Error: the "ace10_LX7HiFi4" core is not in the current Xtensa core registry.

You need to either specify the name of a registered Xtensa core (with
the --xtensa-core option or the XTENSA_CORE environment variable) or
specify a different registry of Xtensa cores (with the --xtensa-system
option or the XTENSA_SYSTEM environment variable).

The following Xtensa cores are available:
        <None>

...in the Xtensa registry directories:
        /home/fredoh/xcc/install/tools/RI-2021.7-linux/XtensaTools/bin/../config

After the fixed the registry problem,

:~/work/zwork/sof$ /home/fredoh/xcc/install/tools/RI-2021.7-linux/XtensaTools/bin/xt-clang --version
XtensaTools-14.07.20210809689958 clang version 10.0.1
Target: xtensa-unknown-unknown
Thread model: single
InstalledDir: /home/fredoh/xcc/install/tools/RI-2021.7-linux/XtensaTools/llvm/bin

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 10, 2023

or specify a different registry of Xtensa cores (with the --xtensa-system
option or the XTENSA_SYSTEM environment variable).

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".

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 10, 2023

SOFCI TEST

1 similar comment
@fredoh9
Copy link
Contributor

fredoh9 commented Feb 12, 2023

SOFCI TEST

@fredoh9
Copy link
Contributor

fredoh9 commented Feb 12, 2023

All build machines have new toolchain, RI-2021.7 and ace10_LX7HiFi4

@fredoh9
Copy link
Contributor

fredoh9 commented Feb 12, 2023

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.

Copy link
Member

@lgirdwood lgirdwood left a 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 ?

@aborisovich
Copy link
Contributor Author

aborisovich commented Feb 14, 2023

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.

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 .

@aborisovich
Copy link
Contributor Author

@aborisovich any ETA for non draft ?

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.

@dcpleung
Copy link
Contributor

zephyrproject-rtos/zephyr#54836 would probably have a higher chance of getting merged sooner.

@aborisovich aborisovich force-pushed the mtl-xtensa-toolchain-upgrade branch from 00deaf7 to f46c2d2 Compare February 14, 2023 21:39
@marc-hb
Copy link
Collaborator

marc-hb commented Feb 14, 2023

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.

@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 -no-pie warnings. Fixing C code is probably more time-consuming.

Once the C code is fixed it's also easier for Zephyr people like @dcpleung to give SOF a quick try.

@aborisovich
Copy link
Contributor Author

zephyrproject-rtos/zephyr#54836 would probably have a higher chance of getting merged sooner.

PRs like those give me joy. Finally those names had been fixed, that's what I've been talking about @marc-hb .

@aborisovich
Copy link
Contributor Author

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.

@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 -no-pie warnings. Fixing C code is probably more time-consuming.

Once the C code is fixed it's also easier for Zephyr people like @dcpleung to give SOF a quick try.

Sure, I'll create new PR which we can instantly merge without any dependencies.

@aborisovich
Copy link
Contributor Author

Created new PR #7096 just for fixing C warnings.

@aborisovich
Copy link
Contributor Author

Ok, got info that our Intel Internal CI has new toolchains set up.
@fredoh9 what is the status of SOF CI? Are we ready there?

@gkbldcig
Copy link
Collaborator

Can one of the admins verify this patch?

@aborisovich aborisovich changed the title board: intel_adsp_ace15_mtpm xtensa toolchain upgrade and enable xt-clang [DO NOT MERGE] board: intel_adsp_ace15_mtpm xtensa toolchain upgrade and enable xt-clang Feb 23, 2023
@aborisovich
Copy link
Contributor Author

@jxstelter can you please confirm this switch to xt-clang and C++17 standard?

Copy link
Collaborator

@softwarecki softwarecki Feb 23, 2023

Choose a reason for hiding this comment

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

verbose by default?

Copy link
Contributor Author

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.

Copy link
Collaborator

@marc-hb marc-hb left a 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

Copy link
Collaborator

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 :-)

@aborisovich
Copy link
Contributor Author

aborisovich commented Feb 27, 2023

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.

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.
I can set a language standard in this PR to C++14, but since we need to upgrade it anyway, and the existing C++ code works with the C++17 standard, I would like to upgrade to it straight away.
When this PR is merged we can also work on the upgrading C to C17 standard (but this will require much more work obviously).

@aborisovich
Copy link
Contributor Author

Still waiting for the MTL full scope test results. We will have a results tomorrow. If no regression proven we can merge.

Andrey Borisovich added 3 commits February 28, 2023 14:53
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>
@aborisovich aborisovich force-pushed the mtl-xtensa-toolchain-upgrade branch from 9ae133d to 780c2ab Compare February 28, 2023 13:54
@aborisovich aborisovich changed the title [DO NOT MERGE] board: intel_adsp_ace15_mtpm xtensa toolchain upgrade and enable xt-clang board: intel_adsp_ace15_mtpm xtensa toolchain upgrade and enable xt-clang Feb 28, 2023
@aborisovich aborisovich marked this pull request as ready for review February 28, 2023 13:54
@aborisovich
Copy link
Contributor Author

aborisovich commented Feb 28, 2023

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 .

@aborisovich
Copy link
Contributor Author

aborisovich commented Feb 28, 2023

Ok all green. sof-ci/jenkins/pr-fw-build & Internal Intel CI System/merge/build used new xt-clang toolchain.
We can merge.

@lgirdwood lgirdwood merged commit b2073f1 into thesofproject:main Feb 28, 2023
@aborisovich aborisovich deleted the mtl-xtensa-toolchain-upgrade branch June 11, 2023 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants