Issue 12954043: code review 12954043: go/build, runtime/cgo: explicitly target ARMv5T (Closed)
Descriptioncmd/go, runtime/cgo: explicitly target ARMv5T
The baseline architecture had been left to the GCC configured
default which can be more accomodating than the rest of the Go
toolchain. This prevented instructions used by the 5g compiler,
like BLX, from being used in GCC compiled assembler code.
Patch Set 1 #Patch Set 2 : diff -r 219bcd5db770 https://code.google.com/p/go #Patch Set 3 : diff -r 219bcd5db770 https://code.google.com/p/go #Patch Set 4 : diff -r 219bcd5db770 https://code.google.com/p/go #Patch Set 5 : diff -r 219bcd5db770 https://code.google.com/p/go #Patch Set 6 : diff -r 3833ddddde2b https://code.google.com/p/go #Patch Set 7 : diff -r 3833ddddde2b https://code.google.com/p/go #Patch Set 8 : diff -r 3833ddddde2b https://code.google.com/p/go #Patch Set 9 : diff -r 3833ddddde2b https://code.google.com/p/go #Patch Set 10 : diff -r 3833ddddde2b https://code.google.com/p/go #Patch Set 11 : diff -r 3833ddddde2b https://code.google.com/p/go #Patch Set 12 : diff -r 1e851b943773 https://code.google.com/p/go #
MessagesTotal messages: 25
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
LGTM. untested, but Carl knows what he is talking about. On 15/08/2013, at 6:14, cshapiro@golang.org wrote: > Reviewers: golang-dev1, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > go/build, runtime/cgo: explicitly target ARMv5T > > The baseline architecture had been left to the GCC configured > default which can be more accomodating than the rest of the Go > toolchain. This prevented instructions used by the 5g compiler, > like BLX, from being used in GCC compiled assembler code. > > Please review this at https://codereview.appspot.com/12954043/ > > Affected files: > M src/cmd/go/build.go > M src/pkg/runtime/cgo/gcc_arm.S > > > Index: src/cmd/go/build.go > =================================================================== > --- a/src/cmd/go/build.go > +++ b/src/cmd/go/build.go > @@ -1837,7 +1837,7 @@ > case "6": > return []string{"-m64"} > case "5": > - return []string{"-marm"} // not thumb > + return []string{"-marm", "-march=armv5t"} // not thumb > } > return nil > } > Index: src/pkg/runtime/cgo/gcc_arm.S > =================================================================== > --- a/src/pkg/runtime/cgo/gcc_arm.S > +++ b/src/pkg/runtime/cgo/gcc_arm.S > @@ -25,12 +25,8 @@ > mov r5, r1 > mov r0, r2 > mov r1, r3 > - // setmg(m, g) > - mov lr, pc > - mov pc, r5 > - // fn() > - mov lr, pc > - mov pc, r4 > + blx r5 // setmg(m, g) > + blx r4 // fn() > pop {r4, r5, r6, r7, r8, r9, r10, r11, ip, pc} > > .globl EXT(__stack_chk_fail_local) > > > -- > > ---You received this message because you are subscribed to the Google Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out.
Sign in to reply to this message.
LGTM Cl desc should begin cmd/go (dir under src)
Sign in to reply to this message.
On Wed, Aug 14, 2013 at 2:22 PM, Russ Cox <rsc@golang.org> wrote: > Cl desc should begin cmd/go (dir under src) I've replaced go/build with cmd/go, which is what I think you want.
Sign in to reply to this message.
Yes thanks
Sign in to reply to this message.
On 2013/08/14 21:14:18, cshapiro wrote: > Hello mailto:golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go LGTM. Thanks. Should 5l (lib.c:hostlink()) also be changed to add -march=armv5t to gcc when external linking?
Sign in to reply to this message.
On 2013/08/14 22:05:52, elias.naur wrote: > LGTM. Thanks. Should 5l (lib.c:hostlink()) also be changed to add -march=armv5t > to gcc when external linking? Maybe not, but I could be wrong. The -march=armv5t flag affects code generation while the flags in hostlink() affect linking. Are we generating code as part of host linking?
Sign in to reply to this message.
On Thursday, August 15, 2013 12:13:04 AM UTC+2, Carl Shapiro wrote: > > On 2013/08/14 22:05:52, elias.naur wrote: > > LGTM. Thanks. Should 5l (lib.c:hostlink()) also be changed to add > -march=armv5t > > to gcc when external linking? > > Maybe not, but I could be wrong. The -march=armv5t flag affects code > generation while the flags in hostlink() affect linking. Are we > generating code as part of host linking? > > https://codereview.appspot.com/12954043/ > Russ knows better, but AFAIK 5l only uses gcc to link not to generate any code. It works as is, I just wondered if the two gcc invocations would be better off in sync.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=0a4959c5402a *** cmd/go, runtime/cgo: explicitly target ARMv5T The baseline architecture had been left to the GCC configured default which can be more accomodating than the rest of the Go toolchain. This prevented instructions used by the 5g compiler, like BLX, from being used in GCC compiled assembler code. R=golang-dev, dave, rsc, elias.naur, cshapiro CC=golang-dev https://codereview.appspot.com/12954043
Sign in to reply to this message.
no, host linking only links
Sign in to reply to this message.
Message was sent while issue was closed.
on some platforms (*BSD/ARM), we only support armv6 or better cpus. why restrict gcc to only using armv5t when armv6 or even armv7-a is being used? sorry i'm late to the discussion, but frankly I don't understand this change. i assume the reason to use blx is to support the stack unwinding code, but we probably shouldn't touch -march option (the user is supposed to setup its toolchain to match his hardware). if we want to use blx, then how about this: .word 0xe12fff35 // blx r5 .word 0xe12fff34 // blx r4 in fact, i think if the gcc defaults to generate code for armv4t, then it's ok Go doesn't build because we really needs at least armv5 (or the user is using the wrong toolchain). If my assumption is correct, i'd like to submit a CL to remove the -march flag. Opinions?
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/10/07 01:15:49, minux wrote: > on some platforms (*BSD/ARM), we only support armv6 or better cpus. > why restrict gcc to only using armv5t when armv6 or even armv7-a is > being used? I am not sure your premise is correct. Have you seen this issue? https://code.google.com/p/go/issues/detail?id=6134 > sorry i'm late to the discussion, but frankly I don't understand this change. > i assume the reason to use blx is to support the stack unwinding code, > but we probably shouldn't touch -march option (the user is supposed to > setup its toolchain to match his hardware). > if we want to use blx, then how about this: > .word 0xe12fff35 // blx r5 > .word 0xe12fff34 // blx r4 What specific harm is done by specifying the -march option? > in fact, i think if the gcc defaults to generate code for armv4t, then it's > ok Go doesn't build because we really needs at least armv5 (or the user > is using the wrong toolchain). I am not sure I am following your reasoning. FreeBSD, NetBSD, and Linux still support ARMv4. As such, their compilers cannot be relied on to assemble the BLX instruction in their default configuration. In fact, this change relates to a build break caused by one of the builders refusing to assemble BLX. Are you suggesting we require FreeBSD, NetBSD, and Linux users to build a custom GCC in order to compile Go? If so, that seems like a lot to ask.
Sign in to reply to this message.
On Oct 7, 2013 1:32 PM, <cshapiro@google.com> wrote: > > On 2013/10/07 01:15:49, minux wrote: >> >> on some platforms (*BSD/ARM), we only support armv6 or better cpus. >> why restrict gcc to only using armv5t when armv6 or even armv7-a is >> being used? > > > I am not sure your premise is correct. > > Have you seen this issue? > > https://code.google.com/p/go/issues/detail?id=6134 yes, i knew it. but how is that issue related to this one? > >> sorry i'm late to the discussion, but frankly I don't understand this > > change. >> >> i assume the reason to use blx is to support the stack unwinding code, >> but we probably shouldn't touch -march option (the user is supposed to >> setup its toolchain to match his hardware). >> if we want to use blx, then how about this: >> .word 0xe12fff35 // blx r5 >> .word 0xe12fff34 // blx r4 > > > What specific harm is done by specifying the -march option? performance, of course. and in general, i think -march option is better left for the user to specify. > >> in fact, i think if the gcc defaults to generate code for armv4t, then > > it's >> >> ok Go doesn't build because we really needs at least armv5 (or the user >> is using the wrong toolchain). > I am not sure I am following your reasoning. FreeBSD, NetBSD, and Linux > still support ARMv4. As such, their compilers cannot be relied on to > assemble the BLX instruction in their default configuration. In fact, > this change relates to a build break caused by one of the builders > refusing to assemble BLX. Are you suggesting we require FreeBSD, > NetBSD, and Linux users to build a custom GCC in order to compile Go? > If so, that seems like a lot to ask. as i proposed, we can workaround the limitation by hand assemble the blx instruction. i don't believe using two blx instruction directly worth the performance impact by only optimizing for armv5t.
Sign in to reply to this message.
for a concrete proposal, see https://codereview.appspot.com/14439047/
Sign in to reply to this message.
On Mon, Oct 7, 2013 at 10:46 AM, minux <minux.ma@gmail.com> wrote: > > https://code.google.com/p/go/issues/detail?id=6134 > yes, i knew it. but how is that issue related to this one? You made the claim that we only support ARMv6 and newer CPUs on the BSDs. As mentioned in that issue, that is not the case. > performance, of course. > and in general, i think -march option is better left for the user to > specify. What harm is done to performance by specifying -march=armv5t ? as i proposed, we can workaround the limitation by hand assemble > the blx instruction. > > i don't believe using two blx instruction directly worth the performance > impact by only optimizing for armv5t. > By not specifying this flag, code generation can target armv4 which is worse. It would help if you could clarify the undesirable effects of specifying armv5t.
Sign in to reply to this message.
On Oct 7, 2013 2:06 PM, "Carl Shapiro" <cshapiro@golang.org> wrote: > > On Mon, Oct 7, 2013 at 10:46 AM, minux <minux.ma@gmail.com> wrote: >> >> > https://code.google.com/p/go/issues/detail?id=6134 >> yes, i knew it. but how is that issue related to this one? > > > You made the claim that we only support ARMv6 and newer CPUs on the BSDs. As mentioned in that issue, that is not the case. i didn't claim to only support armv6 and above, i only said that it's often the case. most armv5 machines don't have enough memory to use go. >> performance, of course. >> and in general, i think -march option is better left for the user to specify. > > > What harm is done to performance by specifying -march=armv5t ? it's like specifying -march=i486 on x86 port. is it acceptable? >> as i proposed, we can workaround the limitation by hand assemble >> the blx instruction. >> >> i don't believe using two blx instruction directly worth the performance impact by only optimizing for armv5t. > > By not specifying this flag, code generation can target armv4 which is worse. as i said earlier, this is an user error. their toolchain is a poor match for their machine. you ignored the major users of arm port, which is linux/arm, and i think most linux/arm users do use correct toolchain, as more and more distributions start offering armv7-a with vfpv3 optimized versions. > It would help if you could clarify the undesirable effects of specifying armv5t. gcc won't use newer instructions introduced in armv6 and above. the instruction scheduling is also very different for armv5t and, say, armv7-a.
Sign in to reply to this message.
On Oct 7, 2013 2:06 PM, "Carl Shapiro" <cshapiro@golang.org> wrote: > It would help if you could clarify the undesirable effects of specifying armv5t. a simple example: void f(void) { asm ("clrex"); } $ gcc -marm -march=armv5t t.c if the user knows what he is doing, why this code should fail to assemble in cgo? did we ever add similar restrictions to x86 cgo?
Sign in to reply to this message.
On Mon, Oct 7, 2013 at 11:22 AM, minux <minux.ma@gmail.com> wrote: > most armv5 machines don't have enough memory to use go. > We have ARMv5 builders. > it's like specifying -march=i486 on x86 port. is it acceptable? > That would probably be harmless. > as i said earlier, this is an user error. > their toolchain is a poor match for their machine. Most users use the toolchain that comes with their operating system distribution. > you ignored the major users of arm port, which is linux/arm, and i think > most linux/arm users do use correct toolchain, > as more and more distributions start offering armv7-a with vfpv3 optimized > versions. > As is, there is so much variance in arm, with four major versions of the architecture (v4 to v7), three different instruction sets (arm, thumb, thumb2), more than four different floating point configurations (vfp, vfp2, vfp3-d16, vfp-d32, etc.), and three calling conventions (arm, arm eabi, arm eabi hf). It is unlikely we are going to cover everyone's sweet spot in one configuration. That aside, the most important thing for Go is to make Go binaries fast. The gcc settings have very limited scope and do not affect 5g compiled code. The benefits of targetting a newer architecture are very limited in this regard. > gcc won't use newer instructions introduced in armv6 and above. > I can't think of an instruction could signficantly benefit the limited amount of gcc compiled code. My imagination could be limited. Can you identify such an instruction and show that its use would cause a signficant performance improvement? > the instruction scheduling is also very different for armv5t and, say, > armv7-a. > Based on my reading of the documentation, the instruction scheduling is specified by -mtune, not by -march. Is this not the case?
Sign in to reply to this message.
On Oct 7, 2013 2:49 PM, "Carl Shapiro" <cshapiro@golang.org> wrote: > > On Mon, Oct 7, 2013 at 11:22 AM, minux <minux.ma@gmail.com> wrote: >> >> most armv5 machines don't have enough memory to use go. > We have ARMv5 builders. did you know we invested how much effort in finding one armv5 system with at least 512MB of memory? >> >> it's like specifying -march=i486 on x86 port. is it acceptable? > That would probably be harmless. of course not. did you even try? as an example, i've observed 25% performance difference on synthesized dhrystone benchmark on netbsd/386. >> >> as i said earlier, this is an user error. >> their toolchain is a poor match for their machine. > > Most users use the toolchain that comes with their operating system distribution. right. those people are not the people who want performance. >> >> you ignored the major users of arm port, which is linux/arm, and i think most linux/arm users do use correct toolchain, >> as more and more distributions start offering armv7-a with vfpv3 optimized versions. > > As is, there is so much variance in arm, with four major versions of the architecture (v4 to v7), three different instruction sets (arm, thumb, thumb2), more than four different floating point configurations (vfp, vfp2, vfp3-d16, vfp-d32, etc.), and three calling conventions (arm, arm eabi, arm eabi hf). It is unlikely we are going to cover everyone's sweet spot in one configuration. this argument is irrelevant, we already can support quite a lot of them, eabi, armv5e and above, arm, w/ or w/o vfp. > > That aside, the most important thing for Go is to make Go binaries fast. The gcc settings have very limited scope and do not affect 5g compiled code. The benefits of targetting a newer architecture are very limited in this regard. ok. let's add -march=pentium-mmx for go 386 port. it will ensure that cgo binaries will support all processors that go supports. >> gcc won't use newer instructions introduced in armv6 and above. > > I can't think of an instruction could signficantly benefit the limited amount of gcc compiled code. My imagination could be limited. Can you identify such an instruction and show that its use would cause a signficant performance improvement? perhaps not, but what if user uses armv6 instructions in his gcc code like the example i gave. >> the instruction scheduling is also very different for armv5t and, say, armv7-a. > > Based on my reading of the documentation, the instruction scheduling is specified by -mtune, not by -march. Is this not the case? of course not. how can you optimize for armv7-a pipeline when you are instructed to use only armv5t instructions? you also ignored my example. could you please do some measurements and then backup your claim that using -march=armv5t doesn't have performance impact compared to -march=armv7-a on cortex-a systems? even if there isn't impact, i don't think we should choose -march flag for the user. if you use -march=armv5t just to be able to assemble blx, why not just hard assemble it? is there any other reason that -march=armv5t is required?
Sign in to reply to this message.
Carl put the -march=armv5t in so that we could compile the blx instruction in gcc_arm.S. Minux is right that this has broader consequences. Specifically, any C code we compile with gcc, including the gcc half of the generated cgo wrappers and any standalone C files included in cgo packages, are being forced to use -march=armv5t as well. For me the big question is this: what is a plausible example where being limited is problematic? I expect most cgo packages to invoke gcc just on the wrappers, with the main library being wrapped already compiled and installed separately, not using the go command. Do you know of Go packages that use cgo with large C files that would suffer from the limited architecture? Of course, the intent of Carl's change was not to impose an upper bound on the architecture but to raise the lower bound: there are builders that default to something even older and rejected the blx instruction. If there is a compelling example being hurt by imposing this upper bound, I have no problem removing it and using .word or a pair of mov instructions in place of the two blx. But I'd like to see the example. Thanks. Russ
Sign in to reply to this message.
On Mon, Oct 7, 2013 at 1:45 PM, Russ Cox <rsc@golang.org> wrote: > Of course, the intent of Carl's change was not to impose an upper bound on > the architecture but to raise the lower bound: there are builders that > default to something even older and rejected the blx instruction. If there > is a compelling example being hurt by imposing this upper bound, I have no > problem removing it and using .word or a pair of mov instructions in place > of the two blx. But I'd like to see the example. It seems CGO provides a way to specify a CFLAGS option following a #cgo directive. This looks like a nice place to put a "-march=native -mtune=native" or the like. I believe the last instance of a flag should override any preceeding ones. In this example, the -march=armv5t should be overridden by -march=native.
Sign in to reply to this message.
On Oct 7, 2013 4:56 PM, "Carl Shapiro" <cshapiro@golang.org> wrote: > > On Mon, Oct 7, 2013 at 1:45 PM, Russ Cox <rsc@golang.org> wrote: >> >> Of course, the intent of Carl's change was not to impose an upper bound on the architecture but to raise the lower bound: there are builders that default to something even older and rejected the blx instruction. If there is a compelling example being hurt by imposing this upper bound, I have no problem removing it and using .word or a pair of mov instructions in place of the two blx. But I'd like to see the example. > > > It seems CGO provides a way to specify a CFLAGS option following a #cgo directive. This looks like a nice place to put a "-march=native -mtune=native" or the like. I believe the last instance of a flag should override any preceeding ones. In this example, the -march=armv5t should be overridden by -march=native. those options are inherently tied to the architecture, or in the case of -march=native, to the os (linux), and using it will make a package nonportable, so i don't expect people to add that to a package. instead, i think most cgo packages just expect the default setting is suitable, thus i oppose to setting -march in the go tool.
Sign in to reply to this message.
On Oct 7, 2013 4:45 PM, "Russ Cox" <rsc@golang.org> wrote: > > Carl put the -march=armv5t in so that we could compile the blx instruction in gcc_arm.S. > > Minux is right that this has broader consequences. Specifically, any C code we compile with gcc, including the gcc half of the generated cgo wrappers and any standalone C files included in cgo packages, are being forced to use -march=armv5t as well. > > For me the big question is this: what is a plausible example where being limited is problematic? > > I expect most cgo packages to invoke gcc just on the wrappers, with the main library being wrapped already compiled and installed separately, not using the go command. Do you know of Go packages that use cgo with large C files that would suffer from the limited architecture? > one major example is sqlite driver (e.g. mattn's) i also know people who integrates leveldb source into a package to take advantage of the recently added cxx compiling support. this usage is not rare, in fact, i myself prefer statically linking the c code so that not only the final executable is self-contained, the build process is also independent. also, this benefits windows users, and that is part of the reason sqlite is now bundling the sqlite3.c source. > Of course, the intent of Carl's change was not to impose an upper bound on the architecture but to raise the lower bound: there are builders that default to something even older and rejected the blx instruction. If there is a compelling example being hurt by imposing this upper bound, I have no problem removing it and using .word or a pair of mov instructions in place of the two blx. But I'd like to see the example. yeah, i understand that intention, too bad gcc doesn't support -mat-least-arch option.
Sign in to reply to this message.
On Mon, Oct 7, 2013 at 1:45 PM, Russ Cox <rsc@golang.org> wrote: > Carl put the -march=armv5t in so that we could compile the blx instruction > in gcc_arm.S. If that is the only reason for it, then I think we could also simply add .arch armv5t to gcc_arm.S. The .arch pseudo-op was added to gas 8 years ago. Ian
Sign in to reply to this message.
On Mon, Oct 7, 2013 at 4:36 PM, Ian Lance Taylor <iant@golang.org> wrote: > If that is the only reason for it, then I think we could also simply > add > > .arch armv5t > > to gcc_arm.S. The .arch pseudo-op was added to gas 8 years ago. > Yes, that is the only reason I added the flag. Thanks for the suggestion. I have uploaded a change for review.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
