Re: [PATCH 3/7] conf/machine/include: Cleanup MIPS tunings to match README

[Mark Hatle <mark.hatle@windriver.com>]

On 4/9/12 3:25 PM, Mark Hatle wrote:
> On 4/9/12 3:06 PM, Andreas Oberritter wrote:
>> On 09.04.2012 17:17, Mark Hatle wrote:
>>> On 4/8/12 4:34 PM, Andreas Oberritter wrote:
>>>> On 07.04.2012 02:10, Mark Hatle wrote:
>>>>> Just ran a local build with the qemumips machine, this is a standard
>>>>> mips32 target.
>>>>>
>
> ...
>
>>>>> So the canonical arch is correct, the mips32 is only the packaging
>>>>> arch.  It was always intended that the packaging arch be used in full on
>>>>> MIPS.  (This will allow us to specify mips32r2, mipsiii, mipsiv, etc as
>>>>> necessary if we expand the mips tunings.)
>>>>
>>>> I don't think such a change should be done only few days before a
>>>> release. Until this patch was applied, the packaging arch has always
>>>> been mipsel, not mips32el. Please, revert or fix this!
>>>
>>> This is easy to change to the previous behavior...  however it was a bug
>>> in the original implementation.
>>>
>>> But again, I stress nothing changed except for the packaging arch... the
>>> way the packages are configured, compiled, installed remain the same,
>>> only the package arch has changed.  The only place that may be affected
>>> by this is the package feed mechanism.
>>
>> I think breaking package feeds in such a way is one of the worst things
>> to do in OE.
>>
>>> To revert to the previous behavior, in the
>>> meta/conf/machine/include/tune-mips.inc:
>>>
>
> ...
>
>>>    TUNE_FEATURES_tune-mips32el = "${TUNE_FEATURES_tune-mipsel} mips32"
>>> -MIPSPKGSFX_VARIANT_tune-mips32el = "mips32el"
>>> +MIPSPKGSFX_VARIANT_tune-mips32el = "mipsel"
>>>    PACKAGE_EXTRA_ARCHS_tune-mips32el = "mipsel mips32el"
>>                                                  ^^^^^^^^
>> I don't think this is correct, in all four cases, because no packages
>> will have that arch.
>>
>
> ...
>
>>> Before I submit this patch though, I would like others to weigh in on
>>> the issue.  This was a mistake in the earlier version of the code.  The
>>> "variant" wasn't be set as it should have been.
>>>
>>> The problem is that if you set the tune to "mips", you get the default
>>> compiler behavior.
>>
>> According to the gcc docs, there is no "mips" ISA name. Valid names are:
>> `mips1', `mips2', `mips3', `mips4', `mips32', `mips32r2', `mips64' and
>> `mips64r2'. Therefore I don't understand why "mips" should default to
>> anything else, if it was an alias for mips32 before.
>>
>
> We have two sets of available tunings:
>
> "mips" and "mips32" tunings.. (add el and -nf variants)
>
> These are -different- tunings and today the only way to notice the difference is
> based on the package arch.  The package arch is NOT the target ISA.  It's an
> arbitrary string "we" have come up with to let people know the architecture, ABI
> and optimizations used in producing specific software.  "mips" indicates that
> it's using the default mips compiler options, whatever those may be.  While
> mips32 says it is specifically tuned to the mips32 architecture settings.
>
> I honestly have no idea what the default compiler settings for mips are, but the
> point is the tunings are different.  If you want the "MIPS" tune, you may not be
> able to run the items compiled with the -march=mips32 option.  We have to have a
> way to reconcile this.

I did some further digging into the GCC configuration.

The default configuration is defined in the various defines:

#define _R3000 1
#define __mips_fpr 32
#define _MIPS_ARCH_MIPS1 1
#define _MIPS_ARCH "mips1"
#define _MIPS_TUNE_MIPS1 1
#define _MIPS_TUNE "mips1"
#define __mips 1
#define _MIPS_ISA _MIPS_ISA_MIPS1

The default is defined for the MIPS1 architecture.

The -march=mips32 changes the above to:

#define __mips__ 1
#define _mips 1
#define mips 1
#define __R3000 1
#define __R3000__ 1
#define R3000 1
#define _R3000 1
#define __mips_fpr 32
#define _MIPS_ARCH_MIPS32 1
#define _MIPS_ARCH "mips32"
#define _MIPS_TUNE_MIPS32 1
#define _MIPS_TUNE "mips32"
#define __mips 32
#define __mips_isa_rev 1
#define _MIPS_ISA _MIPS_ISA_MIPS32

Internally in gcc the different between the two is processor optimizations 
change from the R3000 to the MIPS 4Kc, and PTF_AVOID_BRANCHLIKELY is defined.

    PTF_AVOID_BRANCHLIKELY
         Set if it is usually not profitable to use branch-likely instructions
         for this target, typically because the branches are always predicted
         taken and so incur a large overhead when not taken.

So there will in fact be a difference in the generated binaries.

>>>    However, if you set the tune to mips32, you get
>>> "-march=mips32" added to your CCARGS.  This will produce slightly
>>> different binaries, thus "mips" and mips32" are not equal.
>>
>> Btw, meta/recipes-core/eglibc/eglibc-ld.inc doesn't know about mips32 or
>> mips32el, so this probably broke, too.
>> meta/recipes-devtools/gdb/gdb-common.inc likewise. Do overrides still
>> work, e.g. EXTRA_OECONF_mipsel etc.? How about
>> meta/recipes-qt/qt4/qt4_arch.inc?
>
> Overrides are on the GNU canonical arch (TUNE_ARCH) correct?  If that is the
> case then "mips" or "mipsel" is the canonical arch.  Again, we do NOT use the
> package arch for these settings!
>
> Below are the overrides and related elements from the bitbake.conf file:
>
> OVERRIDES =
> "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}:forcevariable"
> DISTROOVERRIDES ?= "${@d.getVar('DISTRO', True) or ''}"
> MACHINEOVERRIDES ?= "${MACHINE}"
>
> # Used by canadian-cross to handle string conversions on TARGET_ARCH where needed
> TRANSLATED_TARGET_ARCH ??= "${@d.getVar('TARGET_ARCH', True).replace("_", "-")}"
>
> TARGET_ARCH = "${TUNE_ARCH}"
>
> So my reading of this is that, unless overriden somewhere outside of
> bitbake.conf, the override does include the TUNE_ARCH, via the TARGET_ARCH, via
> the TRANSLATED_TARGET_ARCH.
>
>>
>> Whatever the answers are, the most important point is that it's the
>> worst point in time to do such a change. We should rather discuss it
>> after the release, if at all.
>
> In order to resolve this consider the following:
>
> We have two tunes, "mips" and "mips32", the difference being the -march=mips32
> in the later case.
>
> In order to support both tunes, we need to have a way to differentiate between
> them on a package arch basis or we end up in a situation where we have two
> packages with different contents and no way to tell them apart.
>
> In order to reconcile the above, the three primary options are see are:
>
> *) Define only mips or mips32 tune, but not both -- producing "mips" as the
> package arch.  (But then what do we do in the future about mips1, mips2, mips3,
> mips4, mips32r2?)
>
> *) Revert the behavior and have two tunes that produce the identical filename
> package with different contents and deal with this in the future.
>
> *) Keep it as it is now and produce mips and mips32 packages based on the
> specific tunings defined by the user
>
> We have a bug, I believe we need to fix it.. first or third options "fix" the
> bug.. the second option retains the bug to be fixed in the future.
>
> If you have an alternative to the above, I'm interested -- I just really don't
> like the "leave the bug" option.
>
> And just to be extra clear, I consider it a defect if we can produce a package
> with the same name for two different tune settings.. (the exception being the
> hell that is ARM and thumb namings.)
>
> --Mark
>
>> Regards,
>> Andreas
>>
>> _______________________________________________
>> Openembedded-core mailing list
>> Openembedded-core@lists.openembedded.org
>> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
>
>
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core