Patchwork [22/22] feature-arm-thumb: Take ARM_INSTRUCTION_SET into account to decide thumb mode

login
register
mail settings
Submitter Saul Wold
Date Aug. 1, 2011, 7:36 a.m.
Message ID <d383462fb2d94c991d3d67ab07f444b0d6ac2c3a.1312182844.git.sgw@linux.intel.com>
Download mbox | patch
Permalink /patch/8927/
State New, archived
Headers show

Comments

Saul Wold - Aug. 1, 2011, 7:36 a.m.
From: Khem Raj <raj.khem@gmail.com>

This will decouple the compiling in thumb mode from having thumb
capable cores.

Signed-off-by: Khem Raj <raj.khem@gmail.com>
---
 .../conf/machine/include/arm/feature-arm-thumb.inc |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Richard Purdie - Aug. 1, 2011, 1:14 p.m.
On Mon, 2011-08-01 at 00:36 -0700, Saul Wold wrote:
> From: Khem Raj <raj.khem@gmail.com>
> 
> This will decouple the compiling in thumb mode from having thumb
> capable cores.
> 
> Signed-off-by: Khem Raj <raj.khem@gmail.com>
> ---
>  .../conf/machine/include/arm/feature-arm-thumb.inc |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/meta/conf/machine/include/arm/feature-arm-thumb.inc b/meta/conf/machine/include/arm/feature-arm-thumb.inc
> index e7d392e..533eab9 100644
> --- a/meta/conf/machine/include/arm/feature-arm-thumb.inc
> +++ b/meta/conf/machine/include/arm/feature-arm-thumb.inc
> @@ -4,9 +4,11 @@
>  # encoded RISC sub-set. Thumb code is smaller (maybe 70% of the ARM size)
>  # but requires more instructions (140% for 70% smaller code) so may be
>  # slower.
> -TUNEVALID[thumb] = "Use thumb instructions instead of ARM"
> -ARM_THUMB_M_OPT = "${@['-mno-thumb', '-mthumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) == 'thumb']}"
> +TUNEVALID[thumb] = "Use thumb instructions instead of ARM if ARM_INSTRUCTION_SET != arm"
> +ARM_THUMB_M_OPT = "${@['-mthumb', '-mno-thumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) != 'arm']}"
>  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "${ARM_THUMB_M_OPT}", d)}"
> +
> +TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "-mthumb", "-mno-thumb", d)}"
>  OVERRIDES .= "${@bb.utils.contains("TUNE_FEATURES", "thumb", ":thumb", "", d)}"
>  
>  # Note armv7 will hit on armv7a as well

The original change committed into master in my absence is broken. This
patch is just making things even worse and more broken :(

I suspect what people really wanted was:

TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "-mno-thumb", d)}"

Cheers,

Richard
Khem Raj - Aug. 1, 2011, 5:46 p.m.
On (01/08/11 14:14), Richard Purdie wrote:
> On Mon, 2011-08-01 at 00:36 -0700, Saul Wold wrote:
> > From: Khem Raj <raj.khem@gmail.com>
> > 
> > This will decouple the compiling in thumb mode from having thumb
> > capable cores.
> > 
> > Signed-off-by: Khem Raj <raj.khem@gmail.com>
> > ---
> >  .../conf/machine/include/arm/feature-arm-thumb.inc |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/meta/conf/machine/include/arm/feature-arm-thumb.inc b/meta/conf/machine/include/arm/feature-arm-thumb.inc
> > index e7d392e..533eab9 100644
> > --- a/meta/conf/machine/include/arm/feature-arm-thumb.inc
> > +++ b/meta/conf/machine/include/arm/feature-arm-thumb.inc
> > @@ -4,9 +4,11 @@
> >  # encoded RISC sub-set. Thumb code is smaller (maybe 70% of the ARM size)
> >  # but requires more instructions (140% for 70% smaller code) so may be
> >  # slower.
> > -TUNEVALID[thumb] = "Use thumb instructions instead of ARM"
> > -ARM_THUMB_M_OPT = "${@['-mno-thumb', '-mthumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) == 'thumb']}"
> > +TUNEVALID[thumb] = "Use thumb instructions instead of ARM if ARM_INSTRUCTION_SET != arm"
> > +ARM_THUMB_M_OPT = "${@['-mthumb', '-mno-thumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) != 'arm']}"
> >  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "${ARM_THUMB_M_OPT}", d)}"
> > +
> > +TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "-mthumb", "-mno-thumb", d)}"
> >  OVERRIDES .= "${@bb.utils.contains("TUNE_FEATURES", "thumb", ":thumb", "", d)}"
> >  
> >  # Note armv7 will hit on armv7a as well

this patch was wrong anyway. The one from Martin was better which was
pushed to help arm builds going.

> 
> The original change committed into master in my absence is broken.i

This commit is what was pushed e23f9ce928353c9da2b9c4bfa9a1a125a7d160f2
how is it broken ?

> patch is just making things even worse and more broken :(
> 
> I suspect what people really wanted was:
> 
> TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "-mno-thumb", d)}"
> 
> Cheers,
> 
> Richard
> 
> 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Phil Blundell - Aug. 5, 2011, 10:59 a.m.
On Mon, 2011-08-01 at 14:14 +0100, Richard Purdie wrote:
> On Mon, 2011-08-01 at 00:36 -0700, Saul Wold wrote:
> > From: Khem Raj <raj.khem@gmail.com>
> > 
> > This will decouple the compiling in thumb mode from having thumb
> > capable cores.
> > 
> > Signed-off-by: Khem Raj <raj.khem@gmail.com>
> > ---
> >  .../conf/machine/include/arm/feature-arm-thumb.inc |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/meta/conf/machine/include/arm/feature-arm-thumb.inc b/meta/conf/machine/include/arm/feature-arm-thumb.inc
> > index e7d392e..533eab9 100644
> > --- a/meta/conf/machine/include/arm/feature-arm-thumb.inc
> > +++ b/meta/conf/machine/include/arm/feature-arm-thumb.inc
> > @@ -4,9 +4,11 @@
> >  # encoded RISC sub-set. Thumb code is smaller (maybe 70% of the ARM size)
> >  # but requires more instructions (140% for 70% smaller code) so may be
> >  # slower.
> > -TUNEVALID[thumb] = "Use thumb instructions instead of ARM"
> > -ARM_THUMB_M_OPT = "${@['-mno-thumb', '-mthumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) == 'thumb']}"
> > +TUNEVALID[thumb] = "Use thumb instructions instead of ARM if ARM_INSTRUCTION_SET != arm"
> > +ARM_THUMB_M_OPT = "${@['-mthumb', '-mno-thumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) != 'arm']}"
> >  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "${ARM_THUMB_M_OPT}", d)}"
> > +
> > +TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "-mthumb", "-mno-thumb", d)}"
> >  OVERRIDES .= "${@bb.utils.contains("TUNE_FEATURES", "thumb", ":thumb", "", d)}"
> >  
> >  # Note armv7 will hit on armv7a as well
> 
> The original change committed into master in my absence is broken. This
> patch is just making things even worse and more broken :(

What's the current situation with this?  As far as I can tell from the
git log, there doesn't appear to have been any significant change in
this area since last week: the head of feature-arm-thumb.inc is still at
the allegedly-broken commit e23f9ce92.  

Does anybody have a summary of what exactly is broken at the moment and
what is being done about it?

thanks

p.
Richard Purdie - Aug. 5, 2011, 11:39 a.m.
On Fri, 2011-08-05 at 11:59 +0100, Phil Blundell wrote:
> On Mon, 2011-08-01 at 14:14 +0100, Richard Purdie wrote:
> > On Mon, 2011-08-01 at 00:36 -0700, Saul Wold wrote:
> > > From: Khem Raj <raj.khem@gmail.com>
> > > 
> > > This will decouple the compiling in thumb mode from having thumb
> > > capable cores.
> > > 
> > > Signed-off-by: Khem Raj <raj.khem@gmail.com>
> > > ---
> > >  .../conf/machine/include/arm/feature-arm-thumb.inc |    6 ++++--
> > >  1 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/meta/conf/machine/include/arm/feature-arm-thumb.inc b/meta/conf/machine/include/arm/feature-arm-thumb.inc
> > > index e7d392e..533eab9 100644
> > > --- a/meta/conf/machine/include/arm/feature-arm-thumb.inc
> > > +++ b/meta/conf/machine/include/arm/feature-arm-thumb.inc
> > > @@ -4,9 +4,11 @@
> > >  # encoded RISC sub-set. Thumb code is smaller (maybe 70% of the ARM size)
> > >  # but requires more instructions (140% for 70% smaller code) so may be
> > >  # slower.
> > > -TUNEVALID[thumb] = "Use thumb instructions instead of ARM"
> > > -ARM_THUMB_M_OPT = "${@['-mno-thumb', '-mthumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) == 'thumb']}"
> > > +TUNEVALID[thumb] = "Use thumb instructions instead of ARM if ARM_INSTRUCTION_SET != arm"
> > > +ARM_THUMB_M_OPT = "${@['-mthumb', '-mno-thumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) != 'arm']}"
> > >  TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "${ARM_THUMB_M_OPT}", d)}"
> > > +
> > > +TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "-mthumb", "-mno-thumb", d)}"
> > >  OVERRIDES .= "${@bb.utils.contains("TUNE_FEATURES", "thumb", ":thumb", "", d)}"
> > >  
> > >  # Note armv7 will hit on armv7a as well
> > 
> > The original change committed into master in my absence is broken. This
> > patch is just making things even worse and more broken :(
> 
> What's the current situation with this?  As far as I can tell from the
> git log, there doesn't appear to have been any significant change in
> this area since last week: the head of feature-arm-thumb.inc is still at
> the allegedly-broken commit e23f9ce92.  
> 
> Does anybody have a summary of what exactly is broken at the moment and
> what is being done about it?

ARM_THUMB_M_OPT = "${@['-mno-thumb', '-mthumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) == 'thumb']}"
TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "${ARM_THUMB_M_OPT}", d)}"

Master has the above. This means it can set -mthumb for machines that don't have the thumb feature which seems wrong to me. I suspect it should be:

TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "-mno-thumb", d)}"

or if gcc can't cope with that,

TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "", d)}"

instead.

Cheers,

Richard
Phil Blundell - Aug. 5, 2011, 2:34 p.m.
On Fri, 2011-08-05 at 12:39 +0100, Richard Purdie wrote:
> ARM_THUMB_M_OPT = "${@['-mno-thumb', '-mthumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) == 'thumb']}"
> TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "${ARM_THUMB_M_OPT}", d)}"
> 
> Master has the above. This means it can set -mthumb for machines that don't have the thumb feature which seems wrong to me. I suspect it should be:

Agreed, it does seem as though any bb.utils.contains() where the "true"
and "false" arms are identical must be wrong.

> TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "-mno-thumb", d)}"
> 
> or if gcc can't cope with that,
> 
> TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "", d)}"

I think either of those should be fine.

p.
Khem Raj - Aug. 5, 2011, 3:23 p.m.
On Friday, August 05, 2011 03:34:43 PM Phil Blundell wrote:
> On Fri, 2011-08-05 at 12:39 +0100, Richard Purdie wrote:
> > ARM_THUMB_M_OPT = "${@['-mno-thumb',
> > '-mthumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) == 'thumb']}"
> > TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb",
> > "${ARM_THUMB_M_OPT}", "${ARM_THUMB_M_OPT}", d)}"
> > Master has the above. This means it can set -mthumb for machines that 
don't have the thumb feature which seems wrong to me. I suspect it should be:
> Agreed, it does seem as though any bb.utils.contains() where the "true"
> and "false" arms are identical must be wrong.
> 
> > TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb",
> > "${ARM_THUMB_M_OPT}", "-mno-thumb", d)}"
> > 
> > or if gcc can't cope with that,
> > 
> > TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb",
> > "${ARM_THUMB_M_OPT}", "", d)}"
> I think either of those should be fine.
> 

I prefer the latter since it will have one option less in gcc commandline
saving a minute bit of time spent in parsing cmdline.
> p.
> 
> 
> 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Richard Purdie - Aug. 5, 2011, 4:23 p.m.
On Fri, 2011-08-05 at 08:23 -0700, Khem Raj wrote:
> On Friday, August 05, 2011 03:34:43 PM Phil Blundell wrote:
> > On Fri, 2011-08-05 at 12:39 +0100, Richard Purdie wrote:
> > > ARM_THUMB_M_OPT = "${@['-mno-thumb',
> > > '-mthumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) == 'thumb']}"
> > > TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb",
> > > "${ARM_THUMB_M_OPT}", "${ARM_THUMB_M_OPT}", d)}"
> > > Master has the above. This means it can set -mthumb for machines that 
> don't have the thumb feature which seems wrong to me. I suspect it should be:
> > Agreed, it does seem as though any bb.utils.contains() where the "true"
> > and "false" arms are identical must be wrong.
> > 
> > > TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb",
> > > "${ARM_THUMB_M_OPT}", "-mno-thumb", d)}"
> > > 
> > > or if gcc can't cope with that,
> > > 
> > > TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb",
> > > "${ARM_THUMB_M_OPT}", "", d)}"
> > I think either of those should be fine.
> > 
> 
> I prefer the latter since it will have one option less in gcc commandline
> saving a minute bit of time spent in parsing cmdline.

Since nobody sent me a patch I've just merged a patch to do the latter.

Cheers,

Richard

Patch

diff --git a/meta/conf/machine/include/arm/feature-arm-thumb.inc b/meta/conf/machine/include/arm/feature-arm-thumb.inc
index e7d392e..533eab9 100644
--- a/meta/conf/machine/include/arm/feature-arm-thumb.inc
+++ b/meta/conf/machine/include/arm/feature-arm-thumb.inc
@@ -4,9 +4,11 @@ 
 # encoded RISC sub-set. Thumb code is smaller (maybe 70% of the ARM size)
 # but requires more instructions (140% for 70% smaller code) so may be
 # slower.
-TUNEVALID[thumb] = "Use thumb instructions instead of ARM"
-ARM_THUMB_M_OPT = "${@['-mno-thumb', '-mthumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) == 'thumb']}"
+TUNEVALID[thumb] = "Use thumb instructions instead of ARM if ARM_INSTRUCTION_SET != arm"
+ARM_THUMB_M_OPT = "${@['-mthumb', '-mno-thumb'][bb.data.getVar('ARM_INSTRUCTION_SET', d, 1) != 'arm']}"
 TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "${ARM_THUMB_M_OPT}", "${ARM_THUMB_M_OPT}", d)}"
+
+TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "thumb", "-mthumb", "-mno-thumb", d)}"
 OVERRIDES .= "${@bb.utils.contains("TUNE_FEATURES", "thumb", ":thumb", "", d)}"
 
 # Note armv7 will hit on armv7a as well