[x86] Avoid memory form of PUSH/CALL for ATOM.

[x86] Avoid memory form of PUSH/CALL for ATOM.

Committed: https://crrev.com/c63e994166eb7c24d435a3c85eb2af1ebc73cf7f
Cr-Commit-Position: refs/heads/master@{#26163}

[Benedikt Meurer, 2015-01-15 06:01:38]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2015-01-15 06:01:38]

How does that affect performance on non-Atom processors?

[Jakob Kummerow, 2015-01-15 09:43:41]

jkummerow@chromium.org changed reviewers:
+ jkummerow@chromium.org

[Jakob Kummerow, 2015-01-15 09:43:41]

DBC: A patch like this absolutely needs a comment of the form:

// Force pushed values to be in registers, because pushing from memory
// is very slow on $SOME_CPUs.

Because otherwise it's very un-obvious why the implementation is more
restrictive than the hardware allows, and some well-meaning developer might
change it back to reduce register pressure or whatever.

[Weiliang, 2015-01-16 07:03:15]

Hi all, 

Thanks a lot for review. 

As we discussed, I put the atom-specific tuning after ATOM flag.

Could you please have a look?

Thanks
-Weiliang

[Benedikt Meurer, 2015-01-16 11:14:58]

I don't really like the use of fixed ebx, might cause trouble later with native
calling conventions. Can we remove the Call changes to ia32 from this CL?

[Weiliang, 2015-01-19 02:24:48]

On 2015/01/16 11:14:58, Benedikt Meurer wrote:
> I don't really like the use of fixed ebx, might cause trouble later with
native
> calling conventions. Can we remove the Call changes to ia32 from this CL?

Thanks Benedikt. 

Please take a look at the patch set 3 and 4. 

Thanks 
-Weiliang

[Benedikt Meurer, 2015-01-19 05:39:25]

bmeurer@chromium.org changed reviewers:
+ danno@chromium.org, svenpanne@chromium.org
- jkummerow@chromium.org

[Benedikt Meurer, 2015-01-19 05:39:25]

+svenpanne

I think we should keep the CpuFeatures::ATOM bit for now; It's not 100% clean,
but CpuFeatures is in need of a cleanup anyway (Sven was talking about doing
that).

https://codereview.chromium.org/853703002/diff/60001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/853703002/diff/60001/src/x64/assembler-x64.cc...
src/x64/assembler-x64.cc:63: if (cpu.is_atom()) FLAG_intel_atom = true;
Modifying flags this way is a bad idea, and it also defeats the purpose of the
flag completely, because now whenever the CPU class detects an ATOM, the
intel-atom flag will be forcibly true.

[Weiliang, 2015-01-19 05:51:02]

https://codereview.chromium.org/853703002/diff/60001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/853703002/diff/60001/src/x64/assembler-x64.cc...
src/x64/assembler-x64.cc:63: if (cpu.is_atom()) FLAG_intel_atom = true;
On 2015/01/19 05:39:25, Benedikt Meurer wrote:
> Modifying flags this way is a bad idea, and it also defeats the purpose of the
> flag completely, because now whenever the CPU class detects an ATOM, the
> intel-atom flag will be forcibly true.
Maybe I miss something?
The logic here: the intel_atom flag is FALSE by default. It is only true if CPU
detects an ATOM or you require it by using "--intel_atom" flag. The latter eases
the test of ATOM specific code path on non-ATOM devices.

[Benedikt Meurer, 2015-01-19 05:57:23]

https://codereview.chromium.org/853703002/diff/60001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/853703002/diff/60001/src/x64/assembler-x64.cc...
src/x64/assembler-x64.cc:63: if (cpu.is_atom()) FLAG_intel_atom = true;
Yes, but you cannot generally test the backend code independent of the CPU that
you are running on. If you happen to run on something that claims to be an ATOM,
you'll always be forced into the ATOM code path. A similar problem exists for
the ATOM CpuFeatures flag, however. Ideally the --intel-atom flag would forcibly
enable the ATOM code path, while --nointel-atom forcibly disables it, and
autodetection  is only used if neither flag is given. However that's not how our
flags work.

So how about this solution: Add a string flag --mcpu=<cpu> with <cpu> default to
"auto". This can be used in general and is not limited to this ATOM thing.

Sven, danno: WDYT?

[Sven Panne, 2015-01-19 07:42:14]

https://codereview.chromium.org/853703002/diff/60001/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/853703002/diff/60001/src/x64/assembler-x64.cc...
src/x64/assembler-x64.cc:63: if (cpu.is_atom()) FLAG_intel_atom = true;
On 2015/01/19 05:57:22, Benedikt Meurer wrote:
> [...]
> So how about this solution: Add a string flag --mcpu=<cpu> with <cpu> default
to
> "auto". This can be used in general and is not limited to this ATOM thing.
> 
> Sven, danno: WDYT?

I agree with Benedikt: Modifying flags is a no-go for several reasons, adding a
new flag seems to be the way to go. Tiny bikeshedding: I would prefer
--march=<arch> because we talk about architectures, not CPUs, and this is more
in line with the GCC/clang flag names.

[danno, 2015-01-19 07:54:22]

I also prefer specifying core with a selector rather than individual flags,
since they are mutually exclusive. However, I'd prefer "--mcore"/"--mcpu" over
"--march", since we tend to use the term "architecture" everywhere to
distinguish chip manufacturers.

[Weiliang, 2015-01-20 05:12:44]

On 2015/01/19 07:54:22, danno wrote:
> I also prefer specifying core with a selector rather than individual flags,
> since they are mutually exclusive. However, I'd prefer "--mcore"/"--mcpu" over
> "--march", since we tend to use the term "architecture" everywhere to
> distinguish chip manufacturers.

Thanks for the comments. 

As discussed, I keep the CpuFeatures::ATOM bit and introduce "--mcpu" flag as a
general solution.
Could you please take a look?

[Sven Panne, 2015-01-20 07:44:52]

LGTM, but I'd like to hear Benedikt's opinion about the last patch set, too.

[Benedikt Meurer, 2015-01-20 13:57:04]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2015-01-20 13:57:05]

LGTM

[commit-bot: I haz the power, 2015-01-20 13:57:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/853703002/100001

[commit-bot: I haz the power, 2015-01-20 14:59:53]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-20 15:00:07]

Patchset 6 (id:??) landed as
https://crrev.com/c63e994166eb7c24d435a3c85eb2af1ebc73cf7f
Cr-Commit-Position: refs/heads/master@{#26163}