Skip to content
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

Add ARC64 support #327

Merged
merged 1 commit into from
May 5, 2021
Merged

Add ARC64 support #327

merged 1 commit into from
May 5, 2021

Conversation

abrodkin
Copy link
Collaborator

zephyrproject-rtos/crosstool-ng#9 is a prerequisite for this one.
And until zephyrproject-rtos/crosstool-ng#9 is not merged let's use my CT-NG fork so that the SDK is really buildable.

CT_TARGET_VENDOR="zephyr"
CT_BINUTILS_SRC_DEVEL=y
CT_BINUTILS_DEVEL_URL="https://github.com/foss-for-synopsys-dwc-arc-processors/binutils-gdb.git"
CT_BINUTILS_DEVEL_REVISION="9742c8bf3899b833122ce0bfbf3e112132c907cb"
Copy link
Contributor

Choose a reason for hiding this comment

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

We need a set of patches added to the zephyr repo instead of git references like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you sure? We use GCC 10.x releases in Zephyr's SDK, while ARC64 is based on GCC master branch (i.e. what's going to be GCC 11.x at some point). Imagine amount of changes involved (it's not just ARC code).

Moreover, from time to time we re-base our ARC64-related changes on top of upstream new master state and so those bumps will look like yet another huge patch for existing patch. So I'd prefer to use that much cleaner approach until our changes are merged upstream (say in a year or so).

Copy link
Contributor

Choose a reason for hiding this comment

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

So for gcc we need to identify a baseline snapshot of the gcc11 work and than have any additional ARC changes on top of that.

CT_BINUTILS_DEVEL_REVISION="9742c8bf3899b833122ce0bfbf3e112132c907cb"
CT_NEWLIB_SRC_DEVEL=y
CT_NEWLIB_DEVEL_URL="https://github.com/foss-for-synopsys-dwc-arc-processors/newlib.git"
CT_NEWLIB_DEVEL_REVISION="850160f6ba8620a6841ce956ee6c88ecc73399ac"
Copy link
Contributor

Choose a reason for hiding this comment

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

We need the patches for newlib to be against https://github.com/zephyrproject-rtos/newlib-cygwin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same here. I may generate patches, but see my explanation above for GCC.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to utilize the same source base for ALL the architectures as much as possible. We can make an exception for GCC, but see my comment above about having a snapshot of upstream gcc sources + patches on top of that for any additional ARC support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well I think there're many reasons to use the same versions of all components for all architectures in Zephyr. But I'm not sure how feasible that could be? Some need more recent version due to significant improvements & fixes, some prefer to stick to older versions because they are known to work.

And I think all becomes much worse if we decide to use a snapshot of not yet released components. Maybe we need to discuss that with wider audience?

As for the current ARC64 changes I think it's fine to differ from other "more stable/traditional" arches becase:

  1. We may not introduce additional turbulence for other arches switching all to WIP components.
  2. We'll minimize maintenance - only update commit hashes from time to time instead of huge patches
  3. ARC64 will also act as a guinea pig highlighting issues we'll see when we eventually switch to GCC 11, Binutils 2.37 etc
  4. This all is for a limited amount of time - we're interested in upstreaming GCC/Binutils/Newlib ASAP. It's too late for GCC 11 obviously, but GCC 12 might have if not all but at least some ARC64 support I hope.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nashif can chime in and we can discuss in the toolchain working group.

However, I understand what you are saying from a toolchain point of view, but from the Zephyr point of view this can be a huge pain. I'm less concerned about gcc/binutils and more about newlib. As I mentioned on a call we had huge pains when the xtensa newlib was out of sync with all the other zephyr arch's. So we really need newlib to be in sync.

right now I'd lean to saying NOT to include the ARC64 support in the SDK and just have the Zephyr ARC64 support point at an "external" toolchain. (similar to how esp32 works).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess Newlib is the simplest thing among these 3 (GCC, Binutils & Newlib) as it's the smallest code-base and amount of changes is not that huge. Moreover for now we use not very recent Newlib (like v3.0 or v3.1) as a base so it's super easy to generate a cumulative patch for ARC64 and rarely update it as needed. As well as re-base it on top of v3.3 (currently used in Zephyr) or even more up-to-date versions when time comes.

So think again, maybe accept ARC64 SDK ;)

@galak
Copy link
Contributor

galak commented Apr 16, 2021

@abrodkin I've do a straight forward rebase of the ARC patches to newlib on the zephyr newlib tree:

https://github.com/galak/newlib-zephyr/tree/newlib-3.3.0-arc

It looks like the ARC tree is based on 3.1.0. Wondering if you can take a look at that. We need to see if this builds and test it out. Also, there are a few changes in those patches that I wonder about:

  1. libgloss/configure - does this need to be changed?
  2. libgloss/libnosys/warning.h - is this needed?
  3. newlib/libc/machine/configure - does this need to be changed?

@abrodkin
Copy link
Collaborator Author

@abrodkin I've do a straight forward rebase of the ARC patches to newlib on the zephyr newlib tree:

https://github.com/galak/newlib-zephyr/tree/newlib-3.3.0-arc

It looks like the ARC tree is based on 3.1.0. Wondering if you can take a look at that. We need to see if this builds and test it out. Also, there are a few changes in those patches that I wonder about:

  1. libgloss/configure - does this need to be changed?
  2. libgloss/libnosys/warning.h - is this needed?
  3. newlib/libc/machine/configure - does this need to be changed?

Ok I finally made that exercise as well and may easily answer - all those things 1, 2 & 3 are not needed.
So I guess your re-based version is good. And should be very close to what I've just done, see https://github.com/foss-for-synopsys-dwc-arc-processors/newlib/tree/abrodkin-arc64-3.3.0 (though I didn't exclude warning.h change).

@abrodkin abrodkin force-pushed the arc64 branch 4 times, most recently from c0d786d to 5f5b601 Compare April 27, 2021 21:49
@abrodkin
Copy link
Collaborator Author

@galak see my most recent update with:

  1. GCC & Binutils are built from 10.3.0 & 2.35.1 releases correspondingly + cumulative ARC64 patches on top of the upstream tarballs
  2. Newlib comes from your tree: https://github.com/galak/newlib-zephyr/tree/newlib-3.3.0-arc
  3. Temporary patch for CT-NG until my re-spin (ARC64: Add support for arc64 target [again] crosstool-ng#10) is not merged back

With all that I hope we're good to go for an alpha of the SDK to be used at least with ARC64.

@abrodkin
Copy link
Collaborator Author

One last finishing touch was in moving ARC64 patches to a separate folder as in case of Binutils we still don't have ARCv2 & ARCv3 back-ends decoupled, thus source tree for ARC64 is not usable at this point for ARCv2.
You may see what was going on when we tried to use that source tree for ARCv2 (https://buildkite.com/zephyr/sdk-ng/builds/291#11c0165f-3265-4eaa-86a3-b06f56ad01eb/165-1659):

[ERROR]    /tmp/ccEIPjs1.s:85: Error: opcode 'ldb' not supported for target arcem
[ERROR]    /tmp/ccEIPjs1.s:106: Error: opcode 'ldb' not supported for target arcem
[ERROR]    /tmp/ccEIPjs1.s:134: Error: opcode 'ldb' not supported for target arcem

Now that problem is obviously gone.

@galak
Copy link
Contributor

galak commented Apr 28, 2021

Ok I finally made that exercise as well and may easily answer - all those things 1, 2 & 3 are not needed.
So I guess your re-based version is good. And should be very close to what I've just done, see https://github.com/foss-for-synopsys-dwc-arc-processors/newlib/tree/abrodkin-arc64-3.3.0 (though I didn't exclude warning.h change).

Can you submit a PR to https://github.com/zephyrproject-rtos/newlib-cygwin

@galak
Copy link
Contributor

galak commented Apr 28, 2021

1. GCC & Binutils are built from 10.3.0 & 2.35.1 releases correspondingly + cumulative ARC64 patches on top of the upstream tarballs

In the GCC & binutils patches can we drop the changes to testsuite files. Can make it harder to manage other changes in the future.

2. Newlib comes from your tree: https://github.com/galak/newlib-zephyr/tree/newlib-3.3.0-arc

See comment about submitting a PR to the zephyr newlib tree.

3. Temporary patch for CT-NG until my re-spin ([zephyrproject-rtos/crosstool-ng#10](https://github.com/zephyrproject-rtos/crosstool-ng/pull/10)) is not merged back

Zephyr CT-NG should be merged, so should be able to update this now.

@galak
Copy link
Contributor

galak commented Apr 28, 2021

In binutils PR there is a change to bfd/elflink.c, gas/write.c. Can you explain that? Is there a separate patch we can have for that if needed.

@galak
Copy link
Contributor

galak commented Apr 28, 2021

What are the patches in a separate patches-arc64 dir instead of patches/

@galak
Copy link
Contributor

galak commented Apr 28, 2021

GCC patch comments:

  • drop change to .gitignore
  • gcc/ifcvt.c - why this change, if need can we have a separate patch for it

@galak
Copy link
Contributor

galak commented Apr 28, 2021

3.

Ok I finally made that exercise as well and may easily answer - all those things 1, 2 & 3 are not needed.
So I guess your re-based version is good. And should be very close to what I've just done, see https://github.com/foss-for-synopsys-dwc-arc-processors/newlib/tree/abrodkin-arc64-3.3.0 (though I didn't exclude warning.h change).

In your tree I don't see any change to warning.h. Can we reduce the change in libgloss/configure to just the arc support?

@abrodkin
Copy link
Collaborator Author

Ok I finally made that exercise as well and may easily answer - all those things 1, 2 & 3 are not needed.
So I guess your re-based version is good. And should be very close to what I've just done, see https://github.com/foss-for-synopsys-dwc-arc-processors/newlib/tree/abrodkin-arc64-3.3.0 (though I didn't exclude warning.h change).

In your tree I don't see any change to warning.h. Can we reduce the change in libgloss/configure to just the arc support?

Right, I didn't see that funny change back in the day and in fact I used your a bit cleaner tree ;)
If you prefer I may clean-up a bit my tree (so that in matches yours) and then submit a PR against https://github.com/zephyrproject-rtos/newlib-cygwin.

@galak
Copy link
Contributor

galak commented Apr 28, 2021

Right, I didn't see that funny change back in the day and in fact I used your a bit cleaner tree ;)
If you prefer I may clean-up a bit my tree (so that in matches yours) and then submit a PR against https://github.com/zephyrproject-rtos/newlib-cygwin.

I think your cleanups are good, just need to see about libgloss/configure

@abrodkin
Copy link
Collaborator Author

Right, I didn't see that funny change back in the day and in fact I used your a bit cleaner tree ;)
If you prefer I may clean-up a bit my tree (so that in matches yours) and then submit a PR against https://github.com/zephyrproject-rtos/newlib-cygwin.

I think your cleanups are good, just need to see about libgloss/configure

Well, see libgloss/configure.in is as clean as:

--- a/libgloss/configure.in
+++ b/libgloss/configure.in
@@ -38,9 +38,12 @@ case "${target}" in
 	AC_CONFIG_SUBDIRS(aarch64)
 	config_testsuite=true
 	;;
-  arc*-*-*)
+  arc-*-* | arceb-*-*)
 	AC_CONFIG_SUBDIRS(arc)
 	;;
+  arc64-*-*)
+	AC_CONFIG_SUBDIRS(arc64)
+	;;
   epiphany-*-*)
 	AC_CONFIG_SUBDIRS(epiphany)
 	config_testsuite=true

And libgloss/configure is that different just because a bit different version of Autoconf (2.69 vs 2.68) was used for generation from updated libgloss/configure.in.

I understand how ugly libgloss/configure looks like and may try to get Autoconf 2.68 but not sure if it is worth it. Whenever we have yet another change in libgloss/configure.in we'll need to regenerate libgloss/configure and its diff won't be very tiny anyway.

@galak
Copy link
Contributor

galak commented Apr 29, 2021

```diff

And libgloss/configure is that different just because a bit different version of Autoconf (2.69 vs 2.68) was used for generation from updated libgloss/configure.in.
I understand how ugly libgloss/configure looks like and may try to get Autoconf 2.68 but not sure if it is worth it. Whenever we have yet another change in libgloss/configure.in we'll need to regenerate libgloss/configure and its diff won't be very tiny anyway.

When I look at the other commits/changes to libgloss/configure.in they look more like the diff you posted. Can we just do that?

@abrodkin
Copy link
Collaborator Author

@galak see my cleaned-up branch here: https://github.com/foss-for-synopsys-dwc-arc-processors/newlib/tree/abrodkin-arc64-3.3.0

What was done:

  1. Removed warning.h nonsense
  2. libgloss/configure patched manually. Note modified legacy arc taget from arc*-... to arc-... - this is needed to tell arc64 from arc- & arceb, otherwise arc64 will match the first expression :)
    diff --git a/libgloss/configure b/libgloss/configure
    index 0d2918cee..88b5a5cd7 100755
    --- a/libgloss/configure
    +++ b/libgloss/configure
    @@ -667,6 +667,7 @@ CCASFLAGS'
     ac_subdirs_all='doc
     aarch64
     arc
    +arc64
     epiphany
     i386
     m32r
    @@ -2406,9 +2407,13 @@ case "${target}" in
     
    	config_testsuite=true
    	;;
    -  arc*-*-*)
    +  arc-*-* | arceb-*-*)
    	subdirs="$subdirs arc"
     
    +	;;
    +  arc64-*-*)
    +	subdirs="$subdirs arc64"
    +
    	;;
       epiphany-*-*)
    	subdirs="$subdirs epiphany"
  3. Recently introduced libgloss/arc64/hl* stuff was removed as we don't use it in Zephyr while there was some strange build problem and we need to take a closer look at that.

@galak
Copy link
Contributor

galak commented Apr 30, 2021

@galak see my cleaned-up branch here: https://github.com/foss-for-synopsys-dwc-arc-processors/newlib/tree/abrodkin-arc64-3.3.0

What was done:

1. Removed `warning.h` nonsense

2. `libgloss/configure` patched manually. Note modified legacy `arc` taget from `arc*-...` to `arc-...` - this is needed to tell `arc64` from `arc-` & `arceb`, otherwise `arc64` will match the first expression :)
   ```diff
   diff --git a/libgloss/configure b/libgloss/configure
   index 0d2918cee..88b5a5cd7 100755
   --- a/libgloss/configure
   +++ b/libgloss/configure
   @@ -667,6 +667,7 @@ CCASFLAGS'
    ac_subdirs_all='doc
    aarch64
    arc
   +arc64
    epiphany
    i386
    m32r
   @@ -2406,9 +2407,13 @@ case "${target}" in
    
   	config_testsuite=true
   	;;
   -  arc*-*-*)
   +  arc-*-* | arceb-*-*)
   	subdirs="$subdirs arc"
    
   +	;;
   +  arc64-*-*)
   +	subdirs="$subdirs arc64"
   +
   	;;
      epiphany-*-*)
   	subdirs="$subdirs epiphany"
   ```

3. Recently introduced `libgloss/arc64/hl*` stuff was removed as we don't use it in Zephyr while there was some strange build problem and we need to take a closer look at that.

Will take a look when I get a chance, do you want to submit it to https://github.com/zephyrproject-rtos/newlib-cygwin

@abrodkin
Copy link
Collaborator Author

Sure, we may merge that into Zephyr's fork. Do you want it to be kept as a set of patches or just one cumulative patch?

@galak
Copy link
Contributor

galak commented May 3, 2021

Sure, we may merge that into Zephyr's fork. Do you want it to be kept as a set of patches or just one cumulative patch?

Set of patches is best when we have it, so lets leave it that way.

Copy link
Contributor

@galak galak left a comment

Choose a reason for hiding this comment

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

Can you rebase this on #340

@abrodkin abrodkin force-pushed the arc64 branch 3 times, most recently from e5d6669 to 170e1a9 Compare May 4, 2021 21:34
@abrodkin
Copy link
Collaborator Author

abrodkin commented May 4, 2021

Rebased. As well as:

  • Removed not-needed patches for common code in GCC (.gitignore & gcc/ifcvt.c)
  • Moved ARC64 patch for GCC in patches with symlink to patches-arc64
  • Removed reference to my CT-NG fork, since Add ARC64 support crosstool-ng#9 is merged now
    Build-tested for ARC64 tools in the container.

@galak
Copy link
Contributor

galak commented May 5, 2021

@abrodkin I pushed an update here - rebased since I merged the newlib change, and added the symlinks I mentioned.

It looks like gcc fails to build with errors of the form:

[ALL  ]    /tmp/ccnr7oUP.s: Assembler messages:
[ERROR]    /tmp/ccnr7oUP.s:61: Error: opcode 'ldb' not supported for target arcem
[ERROR]    /tmp/ccXRhGII.s:85: Error: opcode 'ldb' not supported for target arcem
[ERROR]    /tmp/ccnr7oUP.s:83: Error: opcode 'ldb' not supported for target arcem
[ERROR]    /tmp/ccXRhGII.s:118: Error: opcode 'ldb' not supported for target arcem
[ERROR]    /tmp/ccnr7oUP.s:112: Error: opcode 'ldb' not supported for target arcem
[ERROR]    /tmp/ccXRhGII.s:145: Error: opcode 'ldb' not supported for target arcem
[ERROR]    /tmp/ccnr7oUP.s:141: Error: opcode 'ldb' not supported for target arcem

@abrodkin
Copy link
Collaborator Author

abrodkin commented May 5, 2021

The problem was in somehow reverted [I'm pretty sure it was once correctly updated] commit of CT-NG, which was pointing to the prior zephyrproject-rtos/crosstool-ng@dded2f6 one, effectively we attempted to use CT-NG which had no idea about ARC64 and so we were building for ARCv2 from Binutils which only support ARCv3, thus that strange GCC errors.

See what we had in Buildkite log:

Building arc64
/workdir/build/build_arc64 /workdir/build
  CLEAN log
  CLEAN build dir
  CONF  defconfig
#
# configuration written to .config
#
  GEN   savedefconfig
[INFO ]  Performing some trivial sanity checks
[INFO ]  Build started 20210505.060154
[INFO ]  Building environment variables
[EXTRA]  Preparing working directories
[EXTRA]  Installing user-supplied crosstool-NG configuration
[EXTRA]  =================================================================
[EXTRA]  Dumping internal crosstool-NG configuration
[EXTRA]    Building a toolchain for:
[EXTRA]      build  = x86_64-pc-linux-gnu
[EXTRA]      host   = x86_64-pc-linux-gnu
[EXTRA]      target = arc-zephyr-elf

Note target is arc-zephyr-elf instead of expected arc64-zephyr-elf.

Now fixed with 8531d19.

@galak
Copy link
Contributor

galak commented May 5, 2021

The problem was in somehow reverted [I'm pretty sure it was once correctly updated] commit of CT-NG, which was pointing to the prior zephyrproject-rtos/crosstool-ng@dded2f6 one, effectively we attempted to use CT-NG which had no idea about ARC64 and so we were building for ARCv2 from Binutils which only support ARCv3, thus that strange GCC errors.

Ah, that makes sense

@abrodkin
Copy link
Collaborator Author

abrodkin commented May 5, 2021

And see, now all got built perfectly fine!

Add patches for GCC and binutils to enable ARC64 support.  We also
update crosstool-ng reference to support building ARC64.

The binutils patches are not a clean and probably break support for
none-ARC platforms currently.  As such we create a unique patch dir
(patches-arc64) to be used when building the ARC64 toolchain.  We
symlink in the gcc and gdb patches and just have a unique binutils
patch for ARC64 in there.

Additionally GDB isn't currently supported on ARC64.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
@galak galak merged commit 0a4322d into zephyrproject-rtos:master May 5, 2021
@abrodkin abrodkin deleted the arc64 branch May 5, 2021 14:47
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.

2 participants