Remove redundant loads in DoCheckMaps

Remove redundant loads in DoCheckMaps
Hoist the loop-invariant load out of the loop and call the other CheckMap function
BUG=none
TEST=none

Committed: https://code.google.com/p/v8/source/detail?r=13200

[danno, 2012-11-07 22:42:41]

http://codereview.chromium.org/11369014/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/11369014/diff/1/src/arm/lithium-codegen-arm.cc...
src/arm/lithium-codegen-arm.cc:4837: __ CompareMap(reg, scratch, map, &success,
mode);
Why not pass in the map register here, too (to avoid extra loads of the map),
since the optimization you applied below applies all callers of this helper
method, too.

[rkrithiv, 2012-12-06 02:02:50]

On 2012/11/07 22:42:41, danno wrote:
> http://codereview.chromium.org/11369014/diff/1/src/arm/lithium-codegen-arm.cc
> File src/arm/lithium-codegen-arm.cc (right):
> 
>
http://codereview.chromium.org/11369014/diff/1/src/arm/lithium-codegen-arm.cc...
> src/arm/lithium-codegen-arm.cc:4837: __ CompareMap(reg, scratch, map,
&success,
> mode);
> Why not pass in the map register here, too (to avoid extra loads of the map),
> since the optimization you applied below applies all callers of this helper
> method, too.

As per your suggestion, I am now passing the map register directly to
LCodeGen::DoCheckMapCommon. 
Thanks!

[danno, 2012-12-07 10:47:11]

https://codereview.chromium.org/11369014/diff/5001/src/arm/lithium-codegen-ar...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/11369014/diff/5001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:4831: void LCodeGen::DoCheckMapCommon(Register
scratch,
can you please rename this to map_reg here and elsewhere to make it clear what
it is?

https://codereview.chromium.org/11369014/diff/5001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:4850: __ ldr(scratch, FieldMemOperand(reg,
HeapObject::kMapOffset));
can you please rename this to map_reg here and elsewhere to make it clear what
it is?

https://codereview.chromium.org/11369014/diff/5001/src/arm/lithium-codegen-arm.h
File src/arm/lithium-codegen-arm.h (right):

https://codereview.chromium.org/11369014/diff/5001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.h:134: void DoCheckMapCommon(Register scratch,
Handle<Map> map,
can you please rename this to map_reg here and elsewhere to make it clear what
it is?

[rkrithiv, 2012-12-10 20:20:24]

On 2012/12/07 10:47:11, danno wrote:
>
https://codereview.chromium.org/11369014/diff/5001/src/arm/lithium-codegen-ar...
> File src/arm/lithium-codegen-arm.cc (right):
> 
>
https://codereview.chromium.org/11369014/diff/5001/src/arm/lithium-codegen-ar...
> src/arm/lithium-codegen-arm.cc:4831: void LCodeGen::DoCheckMapCommon(Register
> scratch,
> can you please rename this to map_reg here and elsewhere to make it clear what
> it is?
> 
>
https://codereview.chromium.org/11369014/diff/5001/src/arm/lithium-codegen-ar...
> src/arm/lithium-codegen-arm.cc:4850: __ ldr(scratch, FieldMemOperand(reg,
> HeapObject::kMapOffset));
> can you please rename this to map_reg here and elsewhere to make it clear what
> it is?
> 
>
https://codereview.chromium.org/11369014/diff/5001/src/arm/lithium-codegen-arm.h
> File src/arm/lithium-codegen-arm.h (right):
> 
>
https://codereview.chromium.org/11369014/diff/5001/src/arm/lithium-codegen-ar...
> src/arm/lithium-codegen-arm.h:134: void DoCheckMapCommon(Register scratch,
> Handle<Map> map,
> can you please rename this to map_reg here and elsewhere to make it clear what
> it is?

The renaming has been done in Patch Set 3. Thanks!

[danno, 2012-12-11 14:12:48]

LGTM. I'll land this for you.

[danno, 2012-12-11 14:16:33]

This doesn't apply cleanly to trunk. Can you please rebase an make sure that it
does? Thanks!

[danno, 2012-12-11 14:16:54]

Sorry... I meant bleeding_edge, not trunk.

[rkrithiv, 2012-12-11 19:45:26]

Sorry about that. Path Set 4 should apply cleanly to the bleeding_edge now.

[Sven Panne, 2012-12-12 08:35:44]

This patch regresses our Octane benchmark by 40% on ARM, so I propose to revert
it unless somebody spots e.g. a simple typo in it quickly.

[Jakob Kummerow, 2012-12-12 12:34:57]

https://chromiumcodereview.appspot.com/11369014/diff/15002/src/arm/lithium-co...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/11369014/diff/15002/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:5055: DoCheckMapCommon(map_reg,
I think there's a missing map load before this call:

__ ldr(map_reg, FieldMemOperand(prototype_reg, HeapObject::kMapOffset));

[danno, 2012-12-12 14:32:39]

If you'd like us to re-land this, please fix the issue and ensure that there is
no regression on SunSpider, Kraken or Octane. Thanks!

[rkrithiv, 2012-12-12 23:50:34]

Sorry I missed that, thanks for pointing it out! Patch 5 has this fixed and does
not show a regression on the benchmarks.

[rkrithiv, 2012-12-20 19:11:44]

Is the latest patch ok? Since this issue was already closed, do I need to push
the corrected patch as a new issue? Please let me know, thanks!