Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2223433002:
[arm] Simplify run-time CPU selection. (Closed)
|
Created:
4 years, 4 months ago by jbramley Modified:
4 years, 3 months ago CC:
Michael Hablich, v8-mips-ports_googlegroups.com, v8-ppc-ports_googlegroups.com, v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[arm] Simplify run-time CPU selection.
This replaces the target-selection options (such as "--enable-vfp3")
with a simpler, absolute "--arm-arch" option. This eliminates inferences
and avoids surprising behaviour in impossible situations (such as
"--enable-vfp3 --no-enable-armv7").
The available options are:
--arm-arch=armv6 ARMv6 + VFPv2
--arm-arch=armv7 ARMv7 + VFPv3-D32 + NEON
--arm-arch=armv7+sudiv ARMv7 + VFPv4-D32 + NEON + SUDIV
--arm-arch=armv8 ARMv8 (+ all of the above)
For now, the default setting is "armv8", which results in behaviour very
similar to the existing defaults.
BUG=v8:5077
Committed: https://crrev.com/76ea8f2f95def189e368b45a990e5f8d4fa8bb27
Cr-Commit-Position: refs/heads/master@{#39004}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Rebase only. #Patch Set 3 : Address review comments. #Patch Set 4 : cl format #
Messages
Total messages: 21 (10 generated)
Description was changed from
==========
[arm] Simplify run-time CPU selection.
This replaces the target-selection options (such as "--enable-vfp3")
with a simpler, absolute "--arm-arch" option. This eliminates inferences
and avoids surprising behaviour in impossible situations (such as
"--enable-vfp3 --no-enable-armv7").
The available options are:
--arm-arch=armv6 ARMv6 + VFPv2
--arm-arch=armv7 ARMv7 + VFPv3-D32 + NEON
--arm-arch=armv7+sudiv ARMv7 + VFPv4-D32 + NEON + SUDIV
--arm-arch=armv8 ARMv8 (+ all of the above)
For now, the default setting is "armv8", which results in behaviour very
similar to the existing defaults.
BUG=
==========
to
==========
[arm] Simplify run-time CPU selection.
This replaces the target-selection options (such as "--enable-vfp3")
with a simpler, absolute "--arm-arch" option. This eliminates inferences
and avoids surprising behaviour in impossible situations (such as
"--enable-vfp3 --no-enable-armv7").
The available options are:
--arm-arch=armv6 ARMv6 + VFPv2
--arm-arch=armv7 ARMv7 + VFPv3-D32 + NEON
--arm-arch=armv7+sudiv ARMv7 + VFPv4-D32 + NEON + SUDIV
--arm-arch=armv8 ARMv8 (+ all of the above)
For now, the default setting is "armv8", which results in behaviour very
similar to the existing defaults.
BUG=v8:5077
==========
jacob.bramley@arm.com changed reviewers: + jochen@chromium.org, machenbach@chromium.org, ulan@chromium.org
This is the first step towards v8:5077. For now, I've tried to avoid changing behaviour, particularly that involving feature probing and implied options from the build system. This patch focuses on replacing the CPU selection options.
lgtm
https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h File src/flag-definitions.h (left): https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h#oldc... src/flag-definitions.h:561: DEFINE_BOOL(enable_vfp3, ENABLE_VFP3_DEFAULT, If we pass this flag, will d8 fail or just warn? We're referencing a few flags in the infrastructure: https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/v8/config... https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/v8/builde... https://cs.chromium.org/chromium/src/v8/tools/run-tests.py?q=%22--novfp3%22&s... Maybe we should discuss the current cases and figure out if they are all useful - or will stay useful.
On 2016/08/05 13:17:12, Michael Achenbach (slow) wrote: > https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h > File src/flag-definitions.h (left): > > https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h#oldc... > src/flag-definitions.h:561: DEFINE_BOOL(enable_vfp3, ENABLE_VFP3_DEFAULT, > If we pass this flag, will d8 fail or just warn? It just warns (using the usual mechanism for unrecognised flags). Making it fail might be tricky but I could look into it. > We're referencing a few flags in the infrastructure: > https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/v8/config... > https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/v8/builde... > https://cs.chromium.org/chromium/src/v8/tools/run-tests.py?q=%22--novfp3%22&s... To make the transition smoother, it might be possible for both sets of flags to co-exist for a while. Also note that "run-test.py --novfp3" doesn't work properly at the moment. It turns off VFPv3 but it's implied by something else so it just gets turned on again. (I was going to fix that in another patch once this new mechanism is in place.) > Maybe we should discuss the current cases and figure out if they are all useful > - or will stay useful. Yep, I'm quite happy to do that. Are there any particular cases you are thinking about?
> > Maybe we should discuss the current cases and figure out if they are all > useful > > - or will stay useful. > > Yep, I'm quite happy to do that. Are there any particular cases you are thinking > about? I think currently all our arm sims (on bots) use armv8, but they should rather test armv7 or armv7+sudiv. This is a bug due to feature_probe being used with arm_sim. Since this is already screwed up right now, this CL isn't blocked on it. For the future, I'd like to limit the number of different builds we need as much as possible, i.e. preferably one for arm_sim. And then run different tests with different arm_arch flags. I assume the snapshot should be smallest (i.e. build with armv6), and then run the simulator with that build for all four supported arm_archs. Or in case this is too far from reality, make two builds armv6 and armv7. Test armv6 only with armv6 and test armv7 with the remaining three. Right now we have these two special simulator cases (probably both doing the wrong thing): armv8-a - passes --enable-armv8 at the command-line. Since we're already using armv8 by default this seems to be a noop. novfp3 - passes --noenable-vfp3 at the command-line. Probably not useful as the snapshot is built with current build flag defaults: armv7+VFPv3-D32 and no neon. Also those should not block this change. But we should focus on getting to a better state.
lgtm with comments https://codereview.chromium.org/2223433002/diff/1/src/arm/assembler-arm.cc File src/arm/assembler-arm.cc (right): https://codereview.chromium.org/2223433002/diff/1/src/arm/assembler-arm.cc#ne... src/arm/assembler-arm.cc:65: fprintf(stderr, "Warning: unrecognised value for --arm-arch ('%s').\n", I suggest we raise an error here since the flag is new. If some bot or dev by misconfiguration (e.g. small typo in the string) runs tests with an unknown arch, we'd (rather silently) test the wrong thing. E.g. the test driver doesn't show stdout of single tests (unless they fail). If they print a warning, nobody detects that. https://codereview.chromium.org/2223433002/diff/1/src/arm/assembler-arm.cc#ne... src/arm/assembler-arm.cc:105: #else Same comment as flag-definitions.h - maybe raise error for wrong configs. Or ignore if a clean up and simplification is coming in future CLs. https://codereview.chromium.org/2223433002/diff/1/src/arm/assembler-arm.cc#ne... src/arm/assembler-arm.cc:139: runtime |= kArmv7WithSudiv; Maybe nest the armv8 case within cpu.has_idiva()? Right now it looks like it could be possible to get cpu.has_idiva() == false, but cpu.architecture() >= 8, but it shouldn't be possible, right? https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h File src/flag-definitions.h (left): https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h#oldc... src/flag-definitions.h:561: DEFINE_BOOL(enable_vfp3, ENABLE_VFP3_DEFAULT, Clarification: Warning is desired so that we can make this change without breaking any obsolete bots. We can update the bots later. https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h#newc... src/flag-definitions.h:128: #if !defined(ARM_TEST_NO_FEATURE_PROBE) || \ Why is only the first case checking for ARM_TEST_NO_FEATURE_PROBE? https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h#newc... src/flag-definitions.h:145: # define ARM_ARCH_DEFAULT "armv6" Suggestion: Maybe better raise an error for incorrect definitions? (Maybe in a separate CL in case we hit that error anywhere and would block this CL). Right now we'd always end up with armv6 for incorrect cases, e.g. if somebody defines ARMV8 but not ARMV7.
On 2016/08/12 10:15:43, machenbach (OOO until Aug 28) wrote:
> I think currently all our arm sims (on bots) use armv8, but they should rather
> test armv7 or armv7+sudiv. This is a bug due to feature_probe being used with
> arm_sim. Since this is already screwed up right now, this CL isn't blocked on
> it.
>
> For the future, I'd like to limit the number of different builds we need as
much
> as possible, i.e. preferably one for arm_sim. And then run different tests
with
> different arm_arch flags. I assume the snapshot should be smallest (i.e. build
> with armv6), and then run the simulator with that build for all four supported
> arm_archs.
>
> Or in case this is too far from reality, make two builds armv6 and armv7. Test
> armv6 only with armv6 and test armv7 with the remaining three.
We should prioritise testing with whatever baseline (snapshots + C code) is most
commonly shipped, then also test with simulated run-time CPU detection for the
four targets. Any of those suggestions are an improvement on the situation
today, though.
> Right now we have these two special simulator cases (probably both doing the
> wrong thing):
> armv8-a - passes --enable-armv8 at the command-line. Since we're already using
> armv8 by default this seems to be a noop.
> novfp3 - passes --noenable-vfp3 at the command-line. Probably not useful as
the
> snapshot is built with current build flag defaults: armv7+VFPv3-D32 and no
neon.
Yes, they are doing the wrong thing. Most notably, V8 doesn't work without VFP*,
so --{no}enable-vfp3 should just be removed anyway. (We do support VFPv2 but we
can just look at ARMv6 vs ARMv7 for that.) Also, the flags are currently broken
such that --noenable-vfp3 doesn't have any effect if --enable-armv8 is set,
which is the default.
* https://groups.google.com/forum/#!topic/v8-dev/92XeXHLWBpw
On 2016/08/12 10:16:37, machenbach (OOO until Aug 28) wrote:
> lgtm with comments
>
> https://codereview.chromium.org/2223433002/diff/1/src/arm/assembler-arm.cc
> File src/arm/assembler-arm.cc (right):
>
>
https://codereview.chromium.org/2223433002/diff/1/src/arm/assembler-arm.cc#ne...
> src/arm/assembler-arm.cc:65: fprintf(stderr, "Warning: unrecognised value for
> --arm-arch ('%s').\n",
> I suggest we raise an error here since the flag is new. If some bot or dev by
> misconfiguration (e.g. small typo in the string) runs tests with an unknown
> arch, we'd (rather silently) test the wrong thing. E.g. the test driver
doesn't
> show stdout of single tests (unless they fail). If they print a warning,
nobody
> detects that.
I agree, an error would be a safer option here. Is there anything more elegant
than CHECK for this purpose? The only other cases I found where these arguments
can fail are handled in d8.cc using Shell::Exit, and that isn't appropriate
here.
>
https://codereview.chromium.org/2223433002/diff/1/src/arm/assembler-arm.cc#ne...
> src/arm/assembler-arm.cc:105: #else
> Same comment as flag-definitions.h - maybe raise error for wrong configs. Or
> ignore if a clean up and simplification is coming in future CLs.
I just put the error in one place. I can duplicate it if you think it would be
useful to do so.
>
https://codereview.chromium.org/2223433002/diff/1/src/arm/assembler-arm.cc#ne...
> src/arm/assembler-arm.cc:139: runtime |= kArmv7WithSudiv;
> Maybe nest the armv8 case within cpu.has_idiva()? Right now it looks like it
> could be possible to get cpu.has_idiva() == false, but cpu.architecture() >=
8,
> but it shouldn't be possible, right?
Indeed, that isn't possible. I'll move the if-block as you suggested.
> https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h
> File src/flag-definitions.h (left):
>
>
https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h#oldc...
> src/flag-definitions.h:561: DEFINE_BOOL(enable_vfp3, ENABLE_VFP3_DEFAULT,
> Clarification: Warning is desired so that we can make this change without
> breaking any obsolete bots. We can update the bots later.
Ok, I've put the old flags back for now, and tried to preserve their behaviour
as far as possible. When they are used, they print a warning. This should keep
old bots happy for the time being.
> https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h
> File src/flag-definitions.h (right):
>
>
https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h#newc...
> src/flag-definitions.h:128: #if !defined(ARM_TEST_NO_FEATURE_PROBE) || \
> Why is only the first case checking for ARM_TEST_NO_FEATURE_PROBE?
If "!defined(ARM_TEST_NO_FEATURE_PROBE)" is true in the first case, the "else"
clauses won't fire. The logic is odd but I'm hoping that we can clean this up
with some build system work.
The idea is that if you set ARM_TEST_NO_FEATURE_PROBE, --arm-arch defaults to
something derived from the build flags. Otherwise, it defaults to "armv8" and we
use run-time detection to get as close to that as possible. I'm not sure if this
logic is what we want but it's how the old logic worked, and I didn't want to
change too much at once.
>
https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h#newc...
> src/flag-definitions.h:145: # define ARM_ARCH_DEFAULT "armv6"
> Suggestion: Maybe better raise an error for incorrect definitions? (Maybe in a
> separate CL in case we hit that error anywhere and would block this CL).
>
> Right now we'd always end up with armv6 for incorrect cases, e.g. if somebody
> defines ARMV8 but not ARMV7.
Agreed. I've added some checks where they make sense.
Still lgtm. I guess this can land now? > https://codereview.chromium.org/2223433002/diff/1/src/flag-definitions.h#newc... > > src/flag-definitions.h:128: #if !defined(ARM_TEST_NO_FEATURE_PROBE) || \ > > Why is only the first case checking for ARM_TEST_NO_FEATURE_PROBE? > > If "!defined(ARM_TEST_NO_FEATURE_PROBE)" is true in the first case, the "else" > clauses won't fire. The logic is odd but I'm hoping that we can clean this up > with some build system work. > > The idea is that if you set ARM_TEST_NO_FEATURE_PROBE, --arm-arch defaults to > something derived from the build flags. Otherwise, it defaults to "armv8" and we > use run-time detection to get as close to that as possible. I'm not sure if this > logic is what we want but it's how the old logic worked, and I didn't want to > change too much at once. Makes sense for non-simulator builds. I assume for the simulator ARM_TEST_NO_FEATURE_PROBE should be always set, as in simulator we always "probe" armv8 otherwise. But this can come in a separate CL.
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22806)
The CQ bit was checked by jacob.bramley@arm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2223433002/#ps60001 (title: "cl format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
[arm] Simplify run-time CPU selection.
This replaces the target-selection options (such as "--enable-vfp3")
with a simpler, absolute "--arm-arch" option. This eliminates inferences
and avoids surprising behaviour in impossible situations (such as
"--enable-vfp3 --no-enable-armv7").
The available options are:
--arm-arch=armv6 ARMv6 + VFPv2
--arm-arch=armv7 ARMv7 + VFPv3-D32 + NEON
--arm-arch=armv7+sudiv ARMv7 + VFPv4-D32 + NEON + SUDIV
--arm-arch=armv8 ARMv8 (+ all of the above)
For now, the default setting is "armv8", which results in behaviour very
similar to the existing defaults.
BUG=v8:5077
==========
to
==========
[arm] Simplify run-time CPU selection.
This replaces the target-selection options (such as "--enable-vfp3")
with a simpler, absolute "--arm-arch" option. This eliminates inferences
and avoids surprising behaviour in impossible situations (such as
"--enable-vfp3 --no-enable-armv7").
The available options are:
--arm-arch=armv6 ARMv6 + VFPv2
--arm-arch=armv7 ARMv7 + VFPv3-D32 + NEON
--arm-arch=armv7+sudiv ARMv7 + VFPv4-D32 + NEON + SUDIV
--arm-arch=armv8 ARMv8 (+ all of the above)
For now, the default setting is "armv8", which results in behaviour very
similar to the existing defaults.
BUG=v8:5077
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
[arm] Simplify run-time CPU selection.
This replaces the target-selection options (such as "--enable-vfp3")
with a simpler, absolute "--arm-arch" option. This eliminates inferences
and avoids surprising behaviour in impossible situations (such as
"--enable-vfp3 --no-enable-armv7").
The available options are:
--arm-arch=armv6 ARMv6 + VFPv2
--arm-arch=armv7 ARMv7 + VFPv3-D32 + NEON
--arm-arch=armv7+sudiv ARMv7 + VFPv4-D32 + NEON + SUDIV
--arm-arch=armv8 ARMv8 (+ all of the above)
For now, the default setting is "armv8", which results in behaviour very
similar to the existing defaults.
BUG=v8:5077
==========
to
==========
[arm] Simplify run-time CPU selection.
This replaces the target-selection options (such as "--enable-vfp3")
with a simpler, absolute "--arm-arch" option. This eliminates inferences
and avoids surprising behaviour in impossible situations (such as
"--enable-vfp3 --no-enable-armv7").
The available options are:
--arm-arch=armv6 ARMv6 + VFPv2
--arm-arch=armv7 ARMv7 + VFPv3-D32 + NEON
--arm-arch=armv7+sudiv ARMv7 + VFPv4-D32 + NEON + SUDIV
--arm-arch=armv8 ARMv8 (+ all of the above)
For now, the default setting is "armv8", which results in behaviour very
similar to the existing defaults.
BUG=v8:5077
Committed: https://crrev.com/76ea8f2f95def189e368b45a990e5f8d4fa8bb27
Cr-Commit-Position: refs/heads/master@{#39004}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/76ea8f2f95def189e368b45a990e5f8d4fa8bb27 Cr-Commit-Position: refs/heads/master@{#39004} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
