Chromium Code Reviews
Help | Chromium Project | Sign in
(96)

Issue 10831049: Improve constant element index access code generation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 8 months ago by danno
Modified:
1 year, 8 months ago
Reviewers:
Jakob
CC:
v8-dev_googlegroups.com
Visibility:
Public.

Description

Improve constant element index access code generation

R=jkummerow@chromium.org

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : Review feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+103 lines, -28 lines) Lint Patch
M src/arm/lithium-arm.cc View 2 chunks +2 lines, -2 lines 0 comments 0 errors Download
M src/arm/lithium-codegen-arm.cc View 1 3 chunks +39 lines, -16 lines 0 comments ? errors Download
M src/hydrogen.cc View 1 chunk +2 lines, -1 line 0 comments 0 errors Download
M src/hydrogen-instructions.h View 1 chunk +17 lines, -5 lines 0 comments ? errors Download
M src/ia32/lithium-codegen-ia32.cc View 1 2 chunks +12 lines, -2 lines 0 comments ? errors Download
M src/x64/lithium-codegen-x64.cc View 1 7 chunks +31 lines, -2 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 3
danno
PTAL
1 year, 8 months ago #1
Jakob
Overall LGTM, but I have comments on the ARM changes. http://codereview.chromium.org/10831049/diff/1/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): http://codereview.chromium.org/10831049/diff/1/src/arm/lithium-codegen-arm.cc#newcode2793 ...
1 year, 8 months ago #2
danno
1 year, 8 months ago #3
https://chromiumcodereview.appspot.com/10831049/diff/1/src/arm/lithium-codege...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/10831049/diff/1/src/arm/lithium-codege...
src/arm/lithium-codegen-arm.cc:2793: if
(instr->hydrogen()->key()->representation().IsTagged()) {
Actually, this does get triggered. It's scary. The HLoadKeyedFastElement
instruction depends on the BoundsCheck instruction. The representation type of
the bounds check isn't important, but it has to be consistent with the index
input to the load. During the phase that eliminates redundant bounds checks, the
key input to the load is replaced by the input to the boundscheck, which can be
tagged. Adding a comment to this effect.

On 2012/07/30 11:58:08, Jakob wrote:
> I don't see how this condition can ever be true. HLoadKeyedFastElements forces
> Integer32 representation for its |key| input.

https://chromiumcodereview.appspot.com/10831049/diff/1/src/arm/lithium-codege...
src/arm/lithium-codegen-arm.cc:2802: Operand(instr->additional_index() <<
kPointerSizeLog2));
I changed this here and in the DoStoreKeyedFastElement code to be consistent (I
copied it here from there originally to make sure the cases were handled
identically).
On 2012/07/30 11:58:08, Jakob wrote:
> What's the benefit of having this addition in generated code here?
> instr->additional_index() is a constant; adding it to FixedArray::kHeaderSize
at
> Hydrogen runtime seems fine to me.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1275:d14800f88434