[turbofan] Avoid embedding type feedback vector into code.

[turbofan] Avoid embedding type feedback vector into code.

R=bmeurer@chromium.org

Committed: https://crrev.com/4ab2a1809408173a460e7bbf06f89bedbd188194
Cr-Commit-Position: refs/heads/master@{#29216}

[Michael Starzinger, 2015-06-22 14:57:55]

mstarzinger@chromium.org changed reviewers:
+ mvstanton@chromium.org

[Michael Starzinger, 2015-06-22 14:57:56]

Benedikt: PTAL.
Michael: FYI.

https://codereview.chromium.org/1198263004/diff/1/src/compiler/ast-graph-buil...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/1198263004/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.h:101: // TODO(mstarzinger): Can we let GVN take
care of that?
Is there particular reason why machine loads are not marked as kIdempotent?

[Benedikt Meurer, 2015-06-23 04:12:38]

https://codereview.chromium.org/1198263004/diff/1/src/compiler/ast-graph-buil...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1198263004/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3565: bool immutable) {
Nit: I'd prefer to not introduce more bool parameters. Can we have an enum here?
Or even better have a separate BuildLoadImmutableObjectField method? These
methods don't share a lot of code anyway.

https://codereview.chromium.org/1198263004/diff/1/src/compiler/ast-graph-buil...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/1198263004/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.h:101: // TODO(mstarzinger): Can we let GVN take
care of that?
On 2015/06/22 14:57:56, Michael Starzinger wrote:
> Is there particular reason why machine loads are not marked as kIdempotent?

I'm not sure if we can mark Load as idempotent. We could try.
But independent of that, we should still canonicalize the feedback vector in the
AstGraphBuilder; this optimization is cheap and local, and w/o it we'd create
two additional nodes per feedback vector user.

[Michael Starzinger, 2015-06-23 07:59:22]

PTAL.

https://codereview.chromium.org/1198263004/diff/1/src/compiler/ast-graph-buil...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1198263004/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.cc:3565: bool immutable) {
On 2015/06/23 04:12:38, Benedikt Meurer wrote:
> Nit: I'd prefer to not introduce more bool parameters. Can we have an enum
here?
> Or even better have a separate BuildLoadImmutableObjectField method? These
> methods don't share a lot of code anyway.

Done. Yeah, I was thinking about that as well. Definitely two methods, not an
enum.

https://codereview.chromium.org/1198263004/diff/1/src/compiler/ast-graph-buil...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/1198263004/diff/1/src/compiler/ast-graph-buil...
src/compiler/ast-graph-builder.h:101: // TODO(mstarzinger): Can we let GVN take
care of that?
On 2015/06/23 04:12:38, Benedikt Meurer wrote:
> On 2015/06/22 14:57:56, Michael Starzinger wrote:
> > Is there particular reason why machine loads are not marked as kIdempotent?
> 
> I'm not sure if we can mark Load as idempotent. We could try.
> But independent of that, we should still canonicalize the feedback vector in
the
> AstGraphBuilder; this optimization is cheap and local, and w/o it we'd create
> two additional nodes per feedback vector user.

Acknowledged. Removed the TODO.

[Benedikt Meurer, 2015-06-23 08:00:42]

lgtm

[Michael Starzinger, 2015-06-23 08:08:53]

The CQ bit was checked by mstarzinger@chromium.org

[commit-bot: I haz the power, 2015-06-23 08:09:06]

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

[commit-bot: I haz the power, 2015-06-23 08:27:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-23 08:27:43]

Patchset 2 (id:??) landed as
https://crrev.com/4ab2a1809408173a460e7bbf06f89bedbd188194
Cr-Commit-Position: refs/heads/master@{#29216}