Add optimized compiler support for generic global loads....

Add optimized compiler support for generic global loads.

These are needed if an access check is required or if we 
read from a global variable that has not beed introduced yet.

[Kevin Millikin (Chromium), 2010-12-09 13:16:51]

Sorry I didn't get to this yesterday.  This will now need to change after my
last change.

It also needs some ARM functions stubbed if you want to keep us building on ARM.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2116
src/hydrogen.cc:2116: void HGraphBuilder::VisitForTypeofValue(Expression* expr)
{
It seems to me that "parent is typeof" is just a flag on a value context, so
this function could be:

void HGraphBuilder::VisitForTypeofValue(Expression* expr) {
  ValueContext for_value(this);
  for_value.mark_as_typeof();
  Visit(expr);
}

That also dodges any problems with "HandleGlobalVariableLoad" (see below).

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2118
src/hydrogen.cc:2118: if (proxy != NULL && !proxy->var()->is_this() &&
proxy->var()->is_global()) {
I think you can just ask the proxy is_this (and maybe is_global?).  If you
follow my suggestion above you don't need to do anything here, though.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2957
src/hydrogen.cc:2957: void
HGraphBuilder::LookupGlobalPropertyCell(VariableProxy* expr,
It seems like it would be cleaner for this function to return a boolean
indicating whether it was found or not, rather than taking a pointer to a flag.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2979
src/hydrogen.cc:2979: void
HGraphBuilder::HandleGlobalVariableLoad(VariableProxy* expr,
We need to restore some sanity to these helper functions.  There are dozens of
"Visit....", "Build....", and "Handle...." in here with no consistent naming
scheme.

I actually inlined this function and removed its definition in my last change
because I was about to put a comment to the effect of "This function should only
be called in tail position as part of visiting...." and just decided to get rid
of the (called once) helper.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2988
src/hydrogen.cc:2988: PushAndAdd(new HLoadGlobalGeneric(expr->var()->name(),
inside_typeof));
Here you could just ask the ast_context if it is a typeof context, which is only
true for value contexts with the typeof bit set.  There's no need to even pass
it to this helper if you set up the context of the subexpression as suggested
above.

[fschneider, 2010-12-22 16:02:33]

Uploaded a new rebased/refactored version.

Please have another look.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2116
src/hydrogen.cc:2116: void HGraphBuilder::VisitForTypeofValue(Expression* expr)
{
On 2010/12/09 13:16:51, Kevin Millikin wrote:
> It seems to me that "parent is typeof" is just a flag on a value context, so
> this function could be:
> 
> void HGraphBuilder::VisitForTypeofValue(Expression* expr) {
>   ValueContext for_value(this);
>   for_value.mark_as_typeof();
>   Visit(expr);
> }
> 
> That also dodges any problems with "HandleGlobalVariableLoad" (see below).

Done.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2118
src/hydrogen.cc:2118: if (proxy != NULL && !proxy->var()->is_this() &&
proxy->var()->is_global()) {
On 2010/12/09 13:16:51, Kevin Millikin wrote:
> I think you can just ask the proxy is_this (and maybe is_global?).  If you
> follow my suggestion above you don't need to do anything here, though.

Done. This part of the code is now gone.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2957
src/hydrogen.cc:2957: void
HGraphBuilder::LookupGlobalPropertyCell(VariableProxy* expr,
On 2010/12/09 13:16:51, Kevin Millikin wrote:
> It seems like it would be cleaner for this function to return a boolean
> indicating whether it was found or not, rather than taking a pointer to a
flag.

I wanted to be able to use the BAILOUT macro from inside, but maybe it's not
worth it.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2979
src/hydrogen.cc:2979: void
HGraphBuilder::HandleGlobalVariableLoad(VariableProxy* expr,
On 2010/12/09 13:16:51, Kevin Millikin wrote:
> We need to restore some sanity to these helper functions.  There are dozens of
> "Visit....", "Build....", and "Handle...." in here with no consistent naming
> scheme.
> 
> I actually inlined this function and removed its definition in my last change
> because I was about to put a comment to the effect of "This function should
only
> be called in tail position as part of visiting...." and just decided to get
rid
> of the (called once) helper.

Done.

http://codereview.chromium.org/5550003/diff/9001/src/hydrogen.cc#newcode2988
src/hydrogen.cc:2988: PushAndAdd(new HLoadGlobalGeneric(expr->var()->name(),
inside_typeof));
On 2010/12/09 13:16:51, Kevin Millikin wrote:
> Here you could just ask the ast_context if it is a typeof context, which is
only
> true for value contexts with the typeof bit set.  There's no need to even pass
> it to this helper if you set up the context of the subexpression as suggested
> above.

Done.