Make FlushICache NOP for Nvidia Denver CPU's.

Make FlushICache NOP for Nvidia Denver CPU's.

Denver supports a coherent cache mechanism. There is no need to clean
the D cache and invalidate I cache. MTS has to check the translation
anytime there is an I cache invalidate and this time can be saved by
making FlushICache a NOP.

The patch improves Octane by roughly 3-4% on Denver.

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

[arajp, 2014-12-15 04:59:39]

arajp@nvidia.com changed reviewers:
+ jfb@chromium.org, rmcilroy@chromium.org

[arajp, 2014-12-15 05:00:30]

On 2014/12/15 04:59:39, arajp wrote:
> mailto:arajp@nvidia.com changed reviewers:
> + mailto:jfb@chromium.org, mailto:rmcilroy@chromium.org

JFB,
This is the same patch I took over from Srikumar since he is on a long vacation.
PTAL.

[Benedikt Meurer, 2014-12-15 05:33:36]

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

[Benedikt Meurer, 2014-12-15 05:33:37]

https://codereview.chromium.org/797233002/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/797233002/diff/1/src/arm/assembler-arm.cc#new...
src/arm/assembler-arm.cc:133: supported_ |= 1u << COHERENT_CACHE;
Nit: Add { and } for block.

https://codereview.chromium.org/797233002/diff/1/src/arm64/assembler-arm64.cc
File src/arm64/assembler-arm64.cc (right):

https://codereview.chromium.org/797233002/diff/1/src/arm64/assembler-arm64.cc...
src/arm64/assembler-arm64.cc:53: supported_ |= 1u << COHERENT_CACHE;
Nit: Add { and } for block.

https://codereview.chromium.org/797233002/diff/1/src/base/cpu.cc
File src/base/cpu.cc (right):

https://codereview.chromium.org/797233002/diff/1/src/base/cpu.cc#newcode303
src/base/cpu.cc:303: variant_(-1),
Can we default to 0 here instead?

[JF, 2014-12-15 16:23:24]

Is it guaranteed that all Denver CPUs will have coherent caches? I'd like to
make sure that this code is still correct when Denver 5 comes out and everyone
forgot that this code is avoiding I$ fushes.

https://codereview.chromium.org/797233002/diff/1/src/arm64/assembler-arm64.cc
File src/arm64/assembler-arm64.cc (right):

https://codereview.chromium.org/797233002/diff/1/src/arm64/assembler-arm64.cc...
src/arm64/assembler-arm64.cc:48: supported_ = 0;
The ARM and x64 version of this file also have:


  // Only use statically determined features for cross compile (snapshot).
  if (cross_compile) return;

Shoud this fie also do this?

https://codereview.chromium.org/797233002/diff/1/src/base/cpu.cc
File src/base/cpu.cc (right):

https://codereview.chromium.org/797233002/diff/1/src/base/cpu.cc#newcode344
src/base/cpu.cc:344: 
You can call CPUID again with 1 in EAX and get the Model and Extended Model
fields, which are similar to the variant field from cpuinfo.

https://codereview.chromium.org/797233002/diff/1/src/base/cpu.cc#newcode405
src/base/cpu.cc:405: variant_ = 0;
The failure value here is 0, and as Benedikt pointed out above he'd like the
default to be 0.

Won't 0 be the same value as the "Denver" variant? It sounds like /proc/cpuinfo
identifies Denver as variant 0? What do other Tegra devices return as variant?

[JF, 2014-12-15 16:27:21]

Could you also update the description of the patch to explain why this is a
significant perf gain (avoid syscall on ARM) and what the performance numbers
look like after this patch.

[rmcilroy, 2014-12-15 16:32:02]

https://codereview.chromium.org/797233002/diff/1/src/arm64/assembler-arm64.cc
File src/arm64/assembler-arm64.cc (right):

https://codereview.chromium.org/797233002/diff/1/src/arm64/assembler-arm64.cc...
src/arm64/assembler-arm64.cc:48: supported_ = 0;
On 2014/12/15 16:23:23, JF wrote:
> The ARM and x64 version of this file also have:
> 
> 
>   // Only use statically determined features for cross compile (snapshot).
>   if (cross_compile) return;
> 
> Shoud this fie also do this?

Yes you will need this here otherwise the snapshot might get generated wrongly.

[arajp, 2014-12-18 12:58:36]

On 2014/12/15 16:23:24, JF wrote:
> Is it guaranteed that all Denver CPUs will have coherent caches? I'd like to
> make sure that this code is still correct when Denver 5 comes out and everyone
> forgot that this code is avoiding I$ fushes.
> 
In the long term, there should be an API to export this information to user
space that is not specific to Denver (like in a vDSO). We want this for all
versions of Denver (implementer 0x4e "N", variant 0x0).
> https://codereview.chromium.org/797233002/diff/1/src/arm64/assembler-arm64.cc
> File src/arm64/assembler-arm64.cc (right):
> 
>
https://codereview.chromium.org/797233002/diff/1/src/arm64/assembler-arm64.cc...
> src/arm64/assembler-arm64.cc:48: supported_ = 0;
> The ARM and x64 version of this file also have:
> 
> 
>   // Only use statically determined features for cross compile (snapshot).
>   if (cross_compile) return;
> 
> Shoud this fie also do this?
> 
> https://codereview.chromium.org/797233002/diff/1/src/base/cpu.cc
> File src/base/cpu.cc (right):
> 
> https://codereview.chromium.org/797233002/diff/1/src/base/cpu.cc#newcode344
> src/base/cpu.cc:344: 
> You can call CPUID again with 1 in EAX and get the Model and Extended Model
> fields, which are similar to the variant field from cpuinfo.
> 
> https://codereview.chromium.org/797233002/diff/1/src/base/cpu.cc#newcode405
> src/base/cpu.cc:405: variant_ = 0;
> The failure value here is 0, and as Benedikt pointed out above he'd like the
> default to be 0.
> 
> Won't 0 be the same value as the "Denver" variant? It sounds like
/proc/cpuinfo
> identifies Denver as variant 0? What do other Tegra devices return as variant?

[arajp, 2014-12-18 13:25:05]

Addressed all your comments.
PTAL

https://codereview.chromium.org/797233002/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/797233002/diff/1/src/arm/assembler-arm.cc#new...
src/arm/assembler-arm.cc:133: supported_ |= 1u << COHERENT_CACHE;
On 2014/12/15 05:33:36, Benedikt Meurer wrote:
> Nit: Add { and } for block.

Done.

https://codereview.chromium.org/797233002/diff/1/src/arm64/assembler-arm64.cc
File src/arm64/assembler-arm64.cc (right):

https://codereview.chromium.org/797233002/diff/1/src/arm64/assembler-arm64.cc...
src/arm64/assembler-arm64.cc:48: supported_ = 0;
On 2014/12/15 16:32:02, rmcilroy wrote:
> On 2014/12/15 16:23:23, JF wrote:
> > The ARM and x64 version of this file also have:
> > 
> > 
> >   // Only use statically determined features for cross compile (snapshot).
> >   if (cross_compile) return;
> > 
> > Shoud this fie also do this?
> 
> Yes you will need this here otherwise the snapshot might get generated
wrongly.

Done.

https://codereview.chromium.org/797233002/diff/1/src/arm64/assembler-arm64.cc...
src/arm64/assembler-arm64.cc:53: supported_ |= 1u << COHERENT_CACHE;
On 2014/12/15 05:33:36, Benedikt Meurer wrote:
> Nit: Add { and } for block.

Done.

https://codereview.chromium.org/797233002/diff/1/src/base/cpu.cc
File src/base/cpu.cc (right):

https://codereview.chromium.org/797233002/diff/1/src/base/cpu.cc#newcode303
src/base/cpu.cc:303: variant_(-1),
On 2014/12/15 05:33:36, Benedikt Meurer wrote:
> Can we default to 0 here instead?

Done.

https://codereview.chromium.org/797233002/diff/1/src/base/cpu.cc#newcode405
src/base/cpu.cc:405: variant_ = 0;
Yes. Denver is identified as 0 in variant.

Recently released Shield Tablet has 0x3 as variant which is a Tegra chip but
doesn't have a Denver CPU.

I changed the failure case to be -1.

[rmcilroy, 2014-12-18 14:05:07]

https://codereview.chromium.org/797233002/diff/20001/src/arm64/assembler-arm6...
File src/arm64/assembler-arm64.cc (right):

https://codereview.chromium.org/797233002/diff/20001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.cc:63: void CpuFeatures::PrintFeatures() { }
Please update PrintFeatures to output COHERENT_CACHE status (as with Arm).

[rmcilroy, 2014-12-18 14:05:40]

On 2014/12/18 14:05:07, rmcilroy wrote:
>
https://codereview.chromium.org/797233002/diff/20001/src/arm64/assembler-arm6...
> File src/arm64/assembler-arm64.cc (right):
> 
>
https://codereview.chromium.org/797233002/diff/20001/src/arm64/assembler-arm6...
> src/arm64/assembler-arm64.cc:63: void CpuFeatures::PrintFeatures() { }
> Please update PrintFeatures to output COHERENT_CACHE status (as with Arm).

Looks good otherwise, but I'll let Benedikt approve.

[JF, 2014-12-18 16:28:10]

You're still missing some of the comments, and the update on the CL description.

[arajp, 2014-12-19 06:12:56]

Addressed the new comment from rmcilroy. PTAL

I used git commit --amend to update the commit message and that did not reflect
here when I next did git cl upload. I had to do git-cl description and git cl
upload and then it worked.

JF,
I just now noticed that I left out one comment of yours. Did you mean to say
updating variant_ through CPUID is a better way than through ExtractField()?
Should I change that also?

Otherwise I think I addressed everyone's comments.

https://codereview.chromium.org/797233002/diff/20001/src/arm64/assembler-arm6...
File src/arm64/assembler-arm64.cc (right):

https://codereview.chromium.org/797233002/diff/20001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.cc:63: void CpuFeatures::PrintFeatures() { }
On 2014/12/18 14:05:07, rmcilroy wrote:
> Please update PrintFeatures to output COHERENT_CACHE status (as with Arm).

Done.

[arajp, 2014-12-19 06:23:19]

PTAL

[Benedikt Meurer, 2014-12-19 06:32:10]

Getting close, final comments.

https://codereview.chromium.org/797233002/diff/100001/src/base/cpu.cc
File src/base/cpu.cc (right):

https://codereview.chromium.org/797233002/diff/100001/src/base/cpu.cc#newcode303
src/base/cpu.cc:303: variant_(0),
The 0 default is not safe as we would default to denver if there is no variant
in cpuinfo, please change it back to -1, sorry for the confusion from my earlier
comment.

https://codereview.chromium.org/797233002/diff/100001/src/base/cpu.h
File src/base/cpu.h (right):

https://codereview.chromium.org/797233002/diff/100001/src/base/cpu.h#newcode51
src/base/cpu.h:51: static const int NV_VARIANT_DENVER = 0x0;
Please rename to NVIDIA_DENVER.

https://codereview.chromium.org/797233002/diff/100001/src/base/cpu.h#newcode51
src/base/cpu.h:51: static const int NV_VARIANT_DENVER = 0x0;
Please rename to NVIDIA_DENVER.

[JF, 2014-12-19 06:51:26]

> I used git commit --amend to update the commit message and that did not
reflect
> here when I next did git cl upload. I had to do git-cl description and git cl
> upload and then it worked.

Oh there's an edit button on the web page that'll work next time.

> I just now noticed that I left out one comment of yours. Did you mean to say
> updating variant_ through CPUID is a better way than through ExtractField()?
> Should I change that also?

I'd rather have all CPUs contain valid data for variant_, not just ARM. CPUID
seems the right way to do it for x86.

Looks good otherwise on my end, but I don't own any V8 code :-)

[Benedikt Meurer, 2014-12-19 08:06:07]

> > I just now noticed that I left out one comment of yours. Did you mean to say
> > updating variant_ through CPUID is a better way than through ExtractField()?
> > Should I change that also?
> 
> I'd rather have all CPUs contain valid data for variant_, not just ARM. CPUID
seems the right way to do it for x86.

I'd say variant is ARM-only for now, as there's no need to have it on x86
currently. We can always extend that later when necessary.

[arajp, 2014-12-19 10:45:32]

PTAL

https://codereview.chromium.org/797233002/diff/100001/src/base/cpu.cc
File src/base/cpu.cc (right):

https://codereview.chromium.org/797233002/diff/100001/src/base/cpu.cc#newcode303
src/base/cpu.cc:303: variant_(0),
On 2014/12/19 06:32:09, Benedikt Meurer wrote:
> The 0 default is not safe as we would default to denver if there is no variant
> in cpuinfo, please change it back to -1, sorry for the confusion from my
earlier
> comment.

Done.

https://codereview.chromium.org/797233002/diff/100001/src/base/cpu.h
File src/base/cpu.h (right):

https://codereview.chromium.org/797233002/diff/100001/src/base/cpu.h#newcode51
src/base/cpu.h:51: static const int NV_VARIANT_DENVER = 0x0;
On 2014/12/19 06:32:09, Benedikt Meurer wrote:
> Please rename to NVIDIA_DENVER.

Done.

[Benedikt Meurer, 2014-12-19 10:48:51]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2014-12-19 10:48:52]

LGTM, thanks.

[commit-bot: I haz the power, 2014-12-19 10:49:27]

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

[commit-bot: I haz the power, 2014-12-19 11:16:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-19 11:16:53]

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