Permalink
Browse files

[Fix] Misc reference handling buglets exposed by enabling inlining

Summary:
 - in f($x, $x &= $y), where f takes its first param by reference, we would
   pass a reference to $x after the assignment, rather than before it (this
   happened independent of inlining).
 - in f(g(x)) if g returned by reference, and f did not take its param by
   reference, f could end up with a reference to the result of g anyway, due to
   copy constructor elision (again, independent of inlining)
 - there was a similar problem when a return-by-reference function got inlined.
   The return value could be destroyed before it was used (resulting in a crash in
   one of the added test cases), or it could just end up refering to the wrong
   thing.
 - in $a = &f(), if f returned by value, and got inlined, $a could end up bound
   to the result of f() (rather than bound to a copy of the result of f())

In order to expose the return-by-reference issue, I had to increase the
inliner's agressiveness. So Ive changed it from a bool, to an int and used it as
the max "cost" for a function.

To minimize extra temps, I added the return value's refness to the FunctionInfo.

Test Plan: fast_tests, slow_tests and slow_tests with AutoInline=5 (there are
still some failures, with the latter - will address separately).

Reviewers: qigao, myang

Reviewed By: myang

CC: ps, mwilliams, myang

Differential Revision: 348039

Task ID: 630786
  • Loading branch information...
1 parent a132a6f commit 7447c5f42c6b93039820ec1aaf2b1d7554af0f31 @markw65 markw65 committed with scottmac Oct 19, 2011
@@ -106,7 +106,7 @@ bool AliasManager::parseOptimizations(const std::string &optimizations,
} else if (opt == "string") {
Option::StringLoopOpts = val;
} else if (opt == "inline") {
- Option::AutoInline = val;
+ Option::AutoInline = val ? 1 : 0;
} else if (opt == "cflow") {
Option::ControlFlow = val;
} else if (opt == "coalesce") {
@@ -115,7 +115,7 @@ bool AliasManager::parseOptimizations(const std::string &optimizations,
val = opt == "all";
Option::EliminateDeadCode = val;
Option::LocalCopyProp = val;
- Option::AutoInline = val;
+ Option::AutoInline = val ? 1 : 0;
Option::ControlFlow = val;
Option::CopyProp = val;
} else {
@@ -2417,7 +2417,7 @@ void AliasManager::gatherInfo(AnalysisResultConstPtr ar, MethodStatementPtr m) {
if (m_inlineAsExpr) {
if (!Option::AutoInline ||
- cost > 1 ||
+ cost > Option::AutoInline ||
func->isVariableArgument() ||
m_variables->getAttribute(VariableTable::ContainsDynamicVariable) ||
m_variables->getAttribute(VariableTable::ContainsExtract) ||
@@ -19,6 +19,7 @@
#include <compiler/expression/constant_expression.h>
#include <compiler/expression/modifier_expression.h>
#include <compiler/expression/expression_list.h>
+#include <compiler/expression/function_call.h>
#include <compiler/analysis/code_error.h>
#include <compiler/statement/statement_list.h>
#include <compiler/analysis/file_scope.h>
@@ -1281,7 +1282,7 @@ void FunctionScope::OutputCPPArguments(ExpressionListPtr params,
// If the implemented type is ref-counted and expected type is variant,
// use VarNR to avoid unnecessary ref-counting because we know
// the actual argument will always has a ref in the callee.
- bool wrap = false;
+ bool wrap = false, wrapv = false;
bool scalar = param->isScalar();
bool isValidFuncIdx = func && i < func->getMaxParamCount();
TypePtr expType(
@@ -1318,12 +1319,30 @@ void FunctionScope::OutputCPPArguments(ExpressionListPtr params,
if (scalar) cg.clearScalarVariant();
}
}
+ } else if (isValidFuncIdx &&
+ !param->hasCPPTemp() &&
+ func->getVariables()->isLvalParam(func->getParamName(i)) &&
+ !param->hasAnyContext(Expression::RefValue|
+ Expression::RefParameter|
+ Expression::InvokeArgument)) {
+ FunctionCallPtr f = dynamic_pointer_cast<FunctionCall>(param);
+ if (f) {
+ if (f->getFuncScope()) {
+ wrapv = f->getFuncScope()->isRefReturn();
+ } else if (!f->getName().empty()) {
+ FunctionInfoPtr info = GetFunctionInfo(f->getName());
+ wrapv = info && info->getMaybeRefReturn();
+ } else {
+ wrapv = true;
+ }
+ if (wrapv) cg_printf("wrap_variant(");
+ }
}
if (wrap) cg_printf("VarNR(");
param->outputCPP(cg, ar);
if (scalar) cg.clearScalarVariant();
ASSERT(!cg.hasScalarVariant());
- if (wrap) {
+ if (wrap || wrapv) {
cg_printf(")");
}
}
@@ -2256,6 +2275,9 @@ void FunctionScope::RecordFunctionInfo(string fname, FunctionScopePtr func) {
if (func->isStatic()) {
info->setMaybeStatic();
}
+ if (func->isRefReturn()) {
+ info->setMaybeRefReturn();
+ }
if (func->isReferenceVariableArgument()) {
info->setRefVarArg(func->getMaxParamCount());
}
@@ -444,7 +444,8 @@ class FunctionScope : public BlockScope,
class FunctionInfo {
public:
- FunctionInfo(int rva = -1) : m_maybeStatic(false), m_refVarArg(rva) { }
+ FunctionInfo(int rva = -1) : m_maybeStatic(false), m_maybeRefReturn(false),
+ m_refVarArg(rva) { }
bool isRefParam(int p) const {
if (m_refVarArg >= 0 && p >= m_refVarArg) return true;
@@ -462,8 +463,12 @@ class FunctionScope : public BlockScope,
void setMaybeStatic() { m_maybeStatic = true; }
bool getMaybeStatic() { return m_maybeStatic; }
+ void setMaybeRefReturn() { m_maybeRefReturn = true; }
+ bool getMaybeRefReturn() { return m_maybeRefReturn; }
+
private:
bool m_maybeStatic; // this could be a static method
+ bool m_maybeRefReturn;
int m_refVarArg; // -1: no ref varargs;
// otherwise, any arg >= m_refVarArg is a reference
std::set<int> m_refParams; // set of ref arg positions
@@ -173,6 +173,7 @@ TypePtr Type::Intersection(AnalysisResultConstPtr ar,
}
bool Type::IsMappedToVariant(TypePtr t) {
+ if (!t) return true;
switch (t->m_kindOf) {
case KindOfBoolean:
case KindOfInt32 :
@@ -34,6 +34,9 @@ class ConstantExpression : public Expression {
DECLARE_BASE_EXPRESSION_VIRTUAL_FUNCTIONS;
ExpressionPtr preOptimize(AnalysisResultConstPtr ar);
+ virtual bool isTemporary() const {
+ return isNull() || isBoolean();
+ }
virtual bool isScalar() const;
virtual bool isLiteralNull() const;
virtual int getLocalEffects() const { return NoEffect; }
@@ -69,6 +69,7 @@ ExpressionPtr Expression::replaceValue(ExpressionPtr rep) {
ExpressionListPtr el(new ExpressionList(getScope(), getLocation(),
ExpressionList::ListKindWrapped));
el->addElement(rep);
+ rep->clearContext(AssignmentRHS);
rep = el;
}
if (rep->is(KindOfSimpleVariable) && !is(KindOfSimpleVariable)) {
@@ -832,18 +833,24 @@ void Expression::preOutputStash(CodeGenerator &cg, AnalysisResultPtr ar,
}
if (dstType) {
+ bool inlinedRefReturn = hasAllContext(LValue|ReturnContext) &&
+ !hasAnyContext(InvokeArgument|RefValue);
bool refParam = hasContext(RefValue) &&
!hasAnyContext(InvokeArgument|AssignmentRHS|ReturnContext) &&
(is(KindOfObjectPropertyExpression) ||
is(KindOfArrayElementExpression));
- if (refParam) {
+ if (inlinedRefReturn) {
+ cg_printf("Variant");
+ } else if (refParam) {
cg_printf("VRefParamValue");
} else {
dstType->outputCPPDecl(cg, ar, getScope());
}
std::string t = genCPPTemp(cg, ar);
const char *ref =
- ((isLvalue || constRef) && !(state & ForceTemp)) ? "&" : "";
+ (!inlinedRefReturn &&
+ (isLvalue || constRef) &&
+ !(state & ForceTemp)) ? "&" : "";
/*
Note that double parens are necessary:
type_name1 tmp27(type_name2(foo));
@@ -871,7 +878,9 @@ void Expression::preOutputStash(CodeGenerator &cg, AnalysisResultPtr ar,
array elements and object properties.
*/
int save = m_context;
- if (refParam) {
+ if (inlinedRefReturn) {
+ cg_printf("strongBind(");
+ } else if (refParam) {
m_context &= ~RefParameter;
} else if (hasContext(RefValue)) {
m_context &= ~RefValue;
@@ -899,6 +908,7 @@ void Expression::preOutputStash(CodeGenerator &cg, AnalysisResultPtr ar,
}
m_context = save;
+ if (inlinedRefReturn) cg_printf(")");
cg_printf("));\n");
if (!refParam && constRef &&
(isLvalue || hasContext(DeepReference) || hasContext(UnsetContext))) {
@@ -539,6 +539,10 @@ bool ExpressionList::preOutputCPP(CodeGenerator &cg, AnalysisResultPtr ar,
!(e->hasContext(LValue) &&
!e->hasAnyContext(RefValue|InvokeArgument))) {
ret = true;
+ } else if (hasContext(RefValue) &&
+ !e->hasAllContext(LValue|ReturnContext) &&
+ !e->hasContext(RefValue)) {
+ ret = true;
}
}
}
@@ -570,28 +574,48 @@ bool ExpressionList::preOutputCPP(CodeGenerator &cg, AnalysisResultPtr ar,
e->setCPPTemp("/**/");
continue;
}
+ /*
+ We inlined a by-value function into the rhs of a by-ref assignment.
+ */
+ bool noRef = hasContext(RefValue) &&
+ !e->hasAllContext(LValue|ReturnContext) &&
+ !e->hasContext(RefValue) &&
+ !e->isTemporary() &&
+ Type::IsMappedToVariant(e->getActualType());
+ /*
+ If we need a non-const reference, but the expression is
+ going to generate a const reference, fix it
+ */
bool lvSwitch =
hasContext(LValue) && !hasAnyContext(RefValue|InvokeArgument) &&
!(e->hasContext(LValue) &&
!e->hasAnyContext(RefValue|InvokeArgument));
- if (lvSwitch || (!i && n > 1)) {
+ if (e->hasAllContext(LValue|ReturnContext) && i + 1 == n) {
+ e->clearContext(ReturnContext);
+ }
+
+ if (noRef || lvSwitch || (!i && n > 1)) {
e->Expression::preOutputStash(cg, ar, state | FixOrder);
if (!(state & FixOrder)) {
cg_printf("id(%s);\n", e->cppTemp().c_str());
}
}
if (e->hasCPPTemp() &&
Type::SameType(e->getType(), getType())) {
- const string &t = e->cppTemp();
+ string t = e->cppTemp();
+ if (noRef) {
+ cg_printf("CVarRef %s_nr = wrap_variant(%s);\n",
+ t.c_str(), t.c_str());
+ t += "_nr";
+ }
if (lvSwitch) {
cg_printf("Variant &%s_lv = const_cast<Variant&>(%s);\n",
t.c_str(), t.c_str());
- setCPPTemp(t + "_lv");
- } else {
- setCPPTemp(t);
+ t += "_lv";
}
+ setCPPTemp(t);
}
}
}
@@ -784,7 +808,15 @@ bool ExpressionList::outputCPPInternal(CodeGenerator &cg,
!(exp->hasContext(LValue) &&
!exp->hasAnyContext(RefValue|InvokeArgument));
if (wrap) cg_printf("const_cast<Variant&>(");
+ bool noRef = !exp->hasCPPTemp() &&
+ hasContext(RefValue) &&
+ !exp->hasAllContext(LValue|ReturnContext) &&
+ !exp->hasContext(RefValue) &&
+ !exp->isTemporary() &&
+ Type::IsMappedToVariant(exp->getActualType());
+ if (noRef) cg_printf("wrap_variant(");
exp->outputCPP(cg, ar);
+ if (noRef) cg_printf(")");
if (wrap) cg_printf(")");
needsComma = true;
}
@@ -465,7 +465,8 @@ ExpressionPtr FunctionCall::inliner(AnalysisResultConstPtr ar,
var->getSymbol()->setHidden();
var->getSymbol()->setUsed();
var->getSymbol()->setReferenced();
- bool ref = m_funcScope->isRefParam(i);
+ bool ref = m_funcScope->isRefParam(i) || arg->hasContext(RefParameter);
+ arg->clearContext(RefParameter);
AssignmentExpressionPtr ae
(new AssignmentExpression(getScope(),
arg->getLocation(),
@@ -503,6 +504,10 @@ ExpressionPtr FunctionCall::inliner(AnalysisResultConstPtr ar,
ExpressionListPtr result_list
(new ExpressionList(getScope(), getLocation(),
ExpressionList::ListKindLeft));
+ if (ret->hasContext(LValue)) {
+ result_list->setContext(LValue);
+ result_list->setContext(ReturnContext);
+ }
result_list->addElement(ret);
result_list->addElement(unset);
(*info.elist)[i] = result_list;
@@ -303,40 +303,69 @@ void SimpleVariable::preOutputStash(CodeGenerator &cg, AnalysisResultPtr ar,
if (hasCPPTemp()) return;
VariableTablePtr vt(getScope()->getVariables());
if (hasContext(InvokeArgument) && !hasContext(AccessContext) &&
- (isLocalExprAltered() || hasEffect()) &&
+ (isLocalExprAltered() || (m_sym && m_sym->isReseated())) &&
!m_globals /* $GLOBALS always has reference semantics */ &&
hasAssignableCPPVariable()) {
- Expression::preOutputStash(cg, ar, state);
- const string &ref_temp = cppTemp();
- ASSERT(!ref_temp.empty());
- const string &copy_temp = genCPPTemp(cg, ar);
- const string &arg_temp = genCPPTemp(cg, ar);
const string &cppName = getAssignableCPPVariable(ar);
ASSERT(!cppName.empty());
- cg_printf("const Variant %s = %s;\n",
- copy_temp.c_str(),
- cppName.c_str());
- cg_printf("const Variant &%s = cit%d->isRef(%d) ? %s : %s;\n",
- arg_temp.c_str(),
- cg.callInfoTop(),
- m_argNum,
- ref_temp.c_str(),
- copy_temp.c_str());
- setCPPTemp(arg_temp);
+ if (m_sym && m_sym->isReseated()) {
+ const string &arg_temp = genCPPTemp(cg, ar);
+ cg.wrapExpressionBegin();
+ cg_printf("const Variant %s = cit%d->isRef(%d) ? "
+ "Variant(strongBind(%s)) : %s;\n",
+ arg_temp.c_str(),
+ cg.callInfoTop(),
+ m_argNum,
+ cppName.c_str(),
+ cppName.c_str());
+
+ setCPPTemp(arg_temp);
+ } else {
+ Expression::preOutputStash(cg, ar, state);
+ const string &ref_temp = cppTemp();
+ ASSERT(!ref_temp.empty());
+ const string &copy_temp = genCPPTemp(cg, ar);
+ const string &arg_temp = genCPPTemp(cg, ar);
+ cg_printf("const Variant %s = %s;\n",
+ copy_temp.c_str(),
+ cppName.c_str());
+ cg_printf("const Variant &%s = cit%d->isRef(%d) ? %s : %s;\n",
+ arg_temp.c_str(),
+ cg.callInfoTop(),
+ m_argNum,
+ ref_temp.c_str(),
+ copy_temp.c_str());
+ setCPPTemp(arg_temp);
+ }
+ return;
+ }
+ if (hasAnyContext(RefValue|RefParameter)) {
+ if (!hasAnyContext(InvokeArgument|AccessContext|
+ AssignmentRHS|ReturnContext) &&
+ !m_globals && (!m_sym || m_sym->isReseated()) &&
+ hasAssignableCPPVariable()) {
+ cg.wrapExpressionBegin();
+ const string &cppName = getAssignableCPPVariable(ar);
+ ASSERT(!cppName.empty());
+ const string &tmp = genCPPTemp(cg, ar);
+ cg_printf("VRefParamValue %s((ref(%s)));\n",
+ tmp.c_str(), cppName.c_str());
+ setCPPTemp(tmp);
+ setContext(NoRefWrapper);
+ }
+ return;
+ } else if (hasContext(LValue)) {
return;
}
- if (getContext() & (LValue|RefValue|RefParameter)) return;
if (!m_alwaysStash && !(state & StashVars)) return;
Expression::preOutputStash(cg, ar, state);
}
bool SimpleVariable::hasAssignableCPPVariable() const {
+ if (!m_this) return true;
+ if (hasAnyContext(OprLValue | AssignmentLHS)) return false;
VariableTablePtr variables = getScope()->getVariables();
- if (m_this) {
- return !hasAnyContext(OprLValue | AssignmentLHS) &&
- variables->getAttribute(VariableTable::ContainsLDynamicVariable);
- }
- return true;
+ return variables->getAttribute(VariableTable::ContainsLDynamicVariable);
}
std::string SimpleVariable::getAssignableCPPVariable(AnalysisResultPtr ar)
View
@@ -217,7 +217,7 @@ bool Option::EliminateDeadCode = true;
bool Option::CopyProp = false;
bool Option::LocalCopyProp = true;
bool Option::StringLoopOpts = true;
-bool Option::AutoInline = false;
+int Option::AutoInline = 0;
bool Option::ControlFlow = true;
bool Option::VariableCoalescing = false;
bool Option::ArrayAccessIdempotent = false;
@@ -426,7 +426,7 @@ void Option::Load(Hdf &config) {
CopyProp = config["CopyProp"].getBool(false);
LocalCopyProp = config["LocalCopyProp"].getBool(true);
StringLoopOpts = config["StringLoopOpts"].getBool(true);
- AutoInline = config["AutoInline"].getBool(false);
+ AutoInline = config["AutoInline"].getInt32(0);
ControlFlow = config["ControlFlow"].getBool(true);
VariableCoalescing = config["VariableCoalescing"].getBool(false);
ArrayAccessIdempotent = config["ArrayAccessIdempotent"].getBool(false);
Oops, something went wrong.

0 comments on commit 7447c5f

Please sign in to comment.