Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(125)

Issue 11678007: SSI implementation. (Closed)

Created:
7 years, 12 months ago by Massi
Modified:
7 years, 2 months ago
Reviewers:
danno
CC:
v8-dev
Visibility:
Public.

Description

SSI implementation.

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+239 lines, -1 line) Patch
M src/arm/lithium-arm.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M src/flag-definitions.h View 1 chunk +2 lines, -0 lines 0 comments Download
M src/hydrogen.h View 4 chunks +28 lines, -0 lines 0 comments Download
M src/hydrogen.cc View 3 chunks +32 lines, -0 lines 0 comments Download
M src/hydrogen-instructions.h View 6 chunks +80 lines, -1 line 0 comments Download
M src/hydrogen-instructions.cc View 2 chunks +73 lines, -0 lines 2 comments Download
M src/ia32/lithium-ia32.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M src/mips/lithium-mips.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M src/x64/lithium-x64.cc View 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 1 (0 generated)
danno
7 years, 12 months ago (2012-12-28 10:42:41 UTC) #1
This generally looks like it's going in the right direction, although I haven't
expected everything in detail yet. 

However, one thing that doesn't feel right is the removal of the SSI nodes after
optimization is complete. I think a better approach is to add a method to HValue
called something like GetBaseValue() which returns "this" for HValue and for SSI
values follows the chain up to what you need. All subsequent passes that need
the "base" value (like the register allocator) should use the GetBaseValue()
method and the graph doesn't need to be re-written to remove SSI. 

Also, I don't know if I'm quite happy with the name SsiDefinition. How about
something that is more consistent with our other value name, e.g. HSSIValue. A
name like this would also makes it clearer what its role is as opposed to a
"plain vanilla" HValue.

https://chromiumcodereview.appspot.com/11678007/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/11678007/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:767: length(), current,
HSsiDefinition::IF_TAGGED_IS_SMI, length());
I don't think we want to handle "SMI-ness" here with SSI here yet. We currently
track SMI-ness using the HType of a HValue, and I don't think it makes sense to
add a new mechanism to do so.  At the very least, SSI nodes that have SMIs
values should return HType::Smi().

https://chromiumcodereview.appspot.com/11678007/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:2269: ValueInfoRelation relation,
I think I would pre just a bit more abstraction here. I think what you want is a
SSIInformation type that abstracts the type of information you can annotate a
value with. You already have two different ones, so I think it makes sense, i.e.
one for comparison relation and one for a type change (i.e. ensuring the
"SMIness" of a value).

Powered by Google App Engine
This is Rietveld 408576698