Towards removing dependency from generic lowering on compilation info.

Remove dependency from generic lowering on compilation info for determining strictness and builtins.

This makes the graphs compositional for inlining (i.e. we can now inline a strict function into a non-strict function, or vice versa).

1) Store strict mode as parameter in StoreNamed/StoreProperty.

R=mstarzinger@chromium.org, titzer@chromium.org

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

[titzer, 2014-08-20 12:54:39]

https://codereview.chromium.org/473263004/diff/1/src/compiler/js-context-spec...
File src/compiler/js-context-specialization.h (right):

https://codereview.chromium.org/473263004/diff/1/src/compiler/js-context-spec...
src/compiler/js-context-specialization.h:19: class JSContextSpecializer {
This class is becoming more general now. I think we should rename this to
JSContextAccessBuilder or something similar.

https://codereview.chromium.org/473263004/diff/1/src/compiler/js-context-spec...
src/compiler/js-context-specialization.h:22: : specialized_(false),
If you make this a parameter, then you can simplify the pipeline a bit. It can
just always call SpecializeToContext(), which will be a no-op if the specializer
is not figured to do specialization.

https://codereview.chromium.org/473263004/diff/1/src/compiler/js-graph.h
File src/compiler/js-graph.h (right):

https://codereview.chromium.org/473263004/diff/1/src/compiler/js-graph.h#newc...
src/compiler/js-graph.h:77: MachineType type = kMachAnyTagged) {
This doesn't seem like the right place for this method...I think it's better to
keep it contained to JSContextSpecialization.

[sigurds, 2014-08-22 14:01:34]

This pulls out the following changes from the initially proposed CL:
(1) Adding strict mode as a parameter
(2) Have pipeline members to configure inlining and context specialization

These changes are orthogonal to the points still open, which I will land in
another CL for sake of clarity. (cf. patchset 3)

https://codereview.chromium.org/473263004/diff/1/src/compiler/js-graph.h
File src/compiler/js-graph.h (right):

https://codereview.chromium.org/473263004/diff/1/src/compiler/js-graph.h#newc...
src/compiler/js-graph.h:77: MachineType type = kMachAnyTagged) {
On 2014/08/20 12:54:38, titzer wrote:
> This doesn't seem like the right place for this method...I think it's better
to
> keep it contained to JSContextSpecialization.

Done.

https://codereview.chromium.org/473263004/diff/100001/tools/run-tests.py
File tools/run-tests.py (right):

https://codereview.chromium.org/473263004/diff/100001/tools/run-tests.py#newc...
tools/run-tests.py:65: "turbofan-generic": ["--turbo-filter=*", "--always-opt",
"--nocontext-specialization"],
Adds a variant with context specialization disabled. Without this, the code I'm
adding is never considered by already existing unit tests since context
specialization is enabled by default and reverts most of the changes.

Please advise if this is a good idea. It will not increase the release checks by
much, but possibly the debug checks.

The optimal thing would be to run only tubofan tests with this mode.

[Michael Starzinger, 2014-08-25 14:15:57]

This conflates two changes, the strict-mode change is non-controversial and I
would have immediately approved it. The refactoring to the pipeline, I have my
quarrels with.

https://codereview.chromium.org/473263004/diff/80002/src/compiler/ast-graph-b...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/473263004/diff/80002/src/compiler/ast-graph-b...
src/compiler/ast-graph-builder.cc:902:
NewNode(javascript()->StoreNamed(info()->strict_mode(), name),
nit: s/info()->strict_mode()/strict_mode()/ here and at all occurences below.

https://codereview.chromium.org/473263004/diff/80002/src/compiler/pipeline.h
File src/compiler/pipeline.h (right):

https://codereview.chromium.org/473263004/diff/80002/src/compiler/pipeline.h#...
src/compiler/pipeline.h:55: bool inlining_;
I don't think this should be carried along in the Pipeline, but should rather be
added to the CompilationInfo (if at all). Can you elaborate why we need this
flag? Will it only ever be used for testing?

[titzer, 2014-08-25 14:31:02]

On 2014/08/25 14:15:57, Michael Starzinger wrote:
> This conflates two changes, the strict-mode change is non-controversial and I
> would have immediately approved it. The refactoring to the pipeline, I have my
> quarrels with.
> 
>
https://codereview.chromium.org/473263004/diff/80002/src/compiler/ast-graph-b...
> File src/compiler/ast-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/473263004/diff/80002/src/compiler/ast-graph-b...
> src/compiler/ast-graph-builder.cc:902:
> NewNode(javascript()->StoreNamed(info()->strict_mode(), name),
> nit: s/info()->strict_mode()/strict_mode()/ here and at all occurences below.
> 
> https://codereview.chromium.org/473263004/diff/80002/src/compiler/pipeline.h
> File src/compiler/pipeline.h (right):
> 
>
https://codereview.chromium.org/473263004/diff/80002/src/compiler/pipeline.h#...
> src/compiler/pipeline.h:55: bool inlining_;
> I don't think this should be carried along in the Pipeline, but should rather
be
> added to the CompilationInfo (if at all). Can you elaborate why we need this
> flag? Will it only ever be used for testing?

After discussion in-person with mstarzinger@, I agree that this flag should be
part of CompilationInfo. It's essentially the dumping ground for all knobs on
the compiler.

[titzer, 2014-08-27 11:48:40]

https://codereview.chromium.org/473263004/diff/150001/test/cctest/compiler/fu...
File test/cctest/compiler/function-tester.h (right):

https://codereview.chromium.org/473263004/diff/150001/test/cctest/compiler/fu...
test/cctest/compiler/function-tester.h:29: enum Flag { kContextSpecializing = 1
<< 0, kInliningEnabled = 2 << 0 };
You can just use the CompilationInfo::Flag here I think.

[sigurds, 2014-08-27 13:02:47]

https://codereview.chromium.org/473263004/diff/150001/test/cctest/compiler/fu...
File test/cctest/compiler/function-tester.h (right):

https://codereview.chromium.org/473263004/diff/150001/test/cctest/compiler/fu...
test/cctest/compiler/function-tester.h:29: enum Flag { kContextSpecializing = 1
<< 0, kInliningEnabled = 2 << 0 };
On 2014/08/27 11:48:40, titzer wrote:
> You can just use the CompilationInfo::Flag here I think.

Done.

[Michael Starzinger, 2014-08-27 13:26:18]

LGTM from my end of comments are addressed.

https://codereview.chromium.org/473263004/diff/170001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/473263004/diff/170001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:902:
NewNode(javascript()->StoreNamed(info()->strict_mode(), name),
nit: Please s/info()->strict_mode()/strict_mode()/ throughout the file.

https://codereview.chromium.org/473263004/diff/170001/test/cctest/compiler/fu...
File test/cctest/compiler/function-tester.h (right):

https://codereview.chromium.org/473263004/diff/170001/test/cctest/compiler/fu...
test/cctest/compiler/function-tester.h:34: USE(flags_);
Why do we need this USE here?

https://codereview.chromium.org/473263004/diff/170001/test/cctest/compiler/fu...
test/cctest/compiler/function-tester.h:49: info.MarkAsContextSpecializing();
If the condition goes over a line-break then please add curly braces.

[sigurds, 2014-08-27 13:48:49]

https://codereview.chromium.org/473263004/diff/170001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/473263004/diff/170001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:902:
NewNode(javascript()->StoreNamed(info()->strict_mode(), name),
On 2014/08/27 13:26:18, Michael Starzinger wrote:
> nit: Please s/info()->strict_mode()/strict_mode()/ throughout the file.

Done.

https://codereview.chromium.org/473263004/diff/170001/test/cctest/compiler/fu...
File test/cctest/compiler/function-tester.h (right):

https://codereview.chromium.org/473263004/diff/170001/test/cctest/compiler/fu...
test/cctest/compiler/function-tester.h:34: USE(flags_);
On 2014/08/27 13:26:18, Michael Starzinger wrote:
> Why do we need this USE here?
The only use is guarded by V8_TURBOFAN_TARGET, so without this use, some arches
won't compile.

https://codereview.chromium.org/473263004/diff/170001/test/cctest/compiler/fu...
test/cctest/compiler/function-tester.h:49: info.MarkAsContextSpecializing();
On 2014/08/27 13:26:18, Michael Starzinger wrote:
> If the condition goes over a line-break then please add curly braces.

Done.

[Michael Starzinger, 2014-08-27 14:06:40]

https://codereview.chromium.org/473263004/diff/170001/test/cctest/compiler/fu...
File test/cctest/compiler/function-tester.h (right):

https://codereview.chromium.org/473263004/diff/170001/test/cctest/compiler/fu...
test/cctest/compiler/function-tester.h:34: USE(flags_);
On 2014/08/27 13:48:49, sigurds wrote:
> On 2014/08/27 13:26:18, Michael Starzinger wrote:
> > Why do we need this USE here?
> The only use is guarded by V8_TURBOFAN_TARGET, so without this use, some
arches
> won't compile.

Acknowledged. Interesting, I didn't know that C-compilers also warn about unused
fields, good to know.

[sigurds, 2014-08-27 14:13:36]

On 2014/08/27 14:06:40, Michael Starzinger wrote:
>
https://codereview.chromium.org/473263004/diff/170001/test/cctest/compiler/fu...
> File test/cctest/compiler/function-tester.h (right):
> 
>
https://codereview.chromium.org/473263004/diff/170001/test/cctest/compiler/fu...
> test/cctest/compiler/function-tester.h:34: USE(flags_);
> On 2014/08/27 13:48:49, sigurds wrote:
> > On 2014/08/27 13:26:18, Michael Starzinger wrote:
> > > Why do we need this USE here?
> > The only use is guarded by V8_TURBOFAN_TARGET, so without this use, some
> arches
> > won't compile.
> 
> Acknowledged. Interesting, I didn't know that C-compilers also warn about
unused
> fields, good to know.

At least clang does warn if private fields are not used: -Wunused-private-field

[sigurds, 2014-08-28 08:39:37]

Committed patchset #11 manually as 23479 (presubmit successful).