Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LocalCopyPropagation pass -- copyprop and elim vars within a function. #33

Merged
merged 1 commit into from
May 2, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions ir/Makefile.am
Original file line number Diff line number Diff line change
@@ -1,32 +1,33 @@
ir_SOURCES += \
ir/base.cpp \
ir/configuration.h \
ir/dbprint.cpp \
ir/dbprint-expression.cpp \
ir/dbprint.h \
ir/dbprint-p4.cpp \
ir/dump.cpp \
ir/expression.cpp \
ir/v1.cpp \
ir/ir.h \
ir/id.h \
ir/ir.cpp \
ir/ir-generated.cpp \
ir/ir.h \
ir/ir-inline.h \
ir/namemap.h \
ir/node.cpp \
ir/node.h \
ir/parameterSubstitution.h \
ir/pass_manager.cpp \
ir/pass_manager.h \
ir/ir.cpp \
ir/substitution.cpp \
ir/substitution.h \
ir/substitutionVisitor.cpp \
ir/substitutionVisitor.h \
ir/type.cpp \
ir/v1.cpp \
ir/vector.h \
ir/visitor.cpp \
ir/visitor.h \
ir/substitution.h \
ir/substitution.cpp \
ir/substitutionVisitor.h \
ir/substitutionVisitor.cpp \
ir/parameterSubstitution.h \
ir/configuration.h
ir/write_context.cpp

ir_DEF_FILES += \
$(srcdir)/ir/base.def \
Expand Down
26 changes: 26 additions & 0 deletions ir/dbprint-p4.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,32 @@ void IR::ActionFunction::dbprint(std::ostream &out) const {
out << unindent << " }";
}

void IR::P4Action::dbprint(std::ostream &out) const {
out << "action " << name << "(";
const char *sep = "";
for (auto &arg : parameters->parameters) {
out << sep << arg.second->direction << ' ' << arg.second->type << ' ' << arg.first;
sep = ", "; }
out << ") {" << indent;
if (body)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one cannot be null.

for (auto &p : *body)
out << endl << p;
out << unindent << " }";
}

void IR::BlockStatement::dbprint(std::ostream &out) const {
out << "{" << indent;
if (components) {
bool first = true;
for (auto &p : *components) {
if (first) {
out << ' ' << p;
first = false;
} else {
out << endl << p; } } }
out << unindent << " }";
}

void IR::ActionProfile::dbprint(std::ostream &out) const { out << "IR::ActionProfile"; }
void IR::ActionSelector::dbprint(std::ostream &out) const { out << "IR::ActionSelector"; }
void IR::V1Table::dbprint(std::ostream &out) const { out << "IR::V1Table " << name; }
Expand Down
12 changes: 9 additions & 3 deletions ir/ir.def
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ class P4Action : Declaration, IGeneralNamespace, IAnnotated {
{ return body->only<IDeclaration>(); }
const Annotations* getAnnotations() const override { return annotations; }
#end
#nodbprint
validate{ body->check_null(); }
}

Expand Down Expand Up @@ -265,7 +264,8 @@ class Declaration_Variable : Declaration, IAnnotated {
#emit
const Annotations* getAnnotations() const override { return annotations; }
#end
#nodbprint
dbprint { Declaration::dbprint(out);
if (initializer) out << " = " << *initializer; }
}

class Declaration_Constant : Declaration, IAnnotated {
Expand Down Expand Up @@ -339,6 +339,13 @@ class IfStatement : Statement {
Statement ifTrue;
NullOK Statement ifFalse;
#nodbprint
visit_children {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think IR classes should depend on Visitors.
Also, does the flow visitor handle all the P4 v1.2 constructs, such as ReturnStatement, ExitStatement, MethodCallStatement, and SwitchStatement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The visit_children function for those nodes should be fixed to support ControlFlowVisitor if they do not.

v.visit(condition, "condition");
auto &clone(v.flow_clone());
v.visit(ifTrue, "ifTrue");
clone.visit(ifFalse, "ifFalse");
v.flow_merge(clone);
}
}

class BlockStatement : Statement, IGeneralNamespace {
Expand All @@ -347,7 +354,6 @@ class BlockStatement : Statement, IGeneralNamespace {
Util::Enumerator<const IDeclaration*>* getDeclarations() const
{ return components->only<IDeclaration>(); }
#end
#nodbprint
}

class MethodCallStatement : Statement {
Expand Down
10 changes: 3 additions & 7 deletions ir/type.def
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,9 @@ class ParameterList : ISimpleNamespace {
const IR::Parameter* getParameter(cstring name) const
{ return parameters.getUnique(name); }
const IR::Parameter* getParameter(unsigned index) const {
BUG_CHECK(index <= size(), "Only %1% parameters; %2% requested", size(), index);
unsigned ix = 0;
auto it = parameters.begin();
for (; ix != index; it++)
++ix;
return it->second;
}
for (auto &param : parameters)
if (0 == index--) return param.second;
BUG("Only %1% parameters; %2% requested", size(), size()+index); }
void add(const Parameter* param)
{ parameters.addUnique(param->name, param); }
const IR::IDeclaration* getDeclByName(cstring name) const
Expand Down
6 changes: 0 additions & 6 deletions ir/visitor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -250,9 +250,3 @@ const IR::Node *Transform::postorder(IR::CLASS *n) {

IRNODE_ALL_SUBCLASSES(DEFINE_VISIT_FUNCTIONS)

bool P4WriteContext::isWrite() {
const Context *ctxt = getContext();
if (auto *prim = dynamic_cast<const IR::Primitive *>(ctxt->node))
return prim->isOutput(ctxt->child_index);
return false;
}
4 changes: 3 additions & 1 deletion ir/visitor.h
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,11 @@ class Backtrack : public virtual Visitor {
virtual bool backtrack(trigger &trig) = 0;
};

namespace P4 { class TypeMap; }

class P4WriteContext : public virtual Visitor {
public:
bool isWrite();
bool isWrite(const P4::TypeMap *typeMap = nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an abstraction violation: the IR definition should not depend on the typeMap.
In general, there should be no dependences on any other namespace than IR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the existence of the P4::TypeMap is the abstraction violation -- I thought about extending the TypeChecking pass to copy the types from the TypeMap into all IR:Expressions (where they belong), but this seemed like a smaller change for now.

Copy link
Contributor

@mbudiu-bfn mbudiu-bfn May 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a smaller change, but why does it have to be in visitor.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it accesses the context, which is a protected part of the Visitor infrastructure (is only valid while visiting the IR in pre/postorder functions)

};

#endif /* _IR_VISITOR_H_ */
29 changes: 29 additions & 0 deletions ir/write_context.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#include "ir.h"
#include "lib/log.h"
#include "frontends/common/typeMap.h"

bool P4WriteContext::isWrite(const P4::TypeMap *typeMap) {
const Context *ctxt = getContext();
if (!ctxt || !ctxt->node)
return false;
if (auto *prim = ctxt->node->to<IR::Primitive>())
return prim->isOutput(ctxt->child_index);
if (ctxt->node->is<IR::AssignmentStatement>())
return ctxt->child_index == 0;
if (ctxt->node->is<IR::Vector<IR::Expression>>()) {
if (!ctxt->parent || !ctxt->parent->node)
return false;
if (auto *mc = ctxt->parent->node->to<IR::MethodCallExpression>()) {
auto type = dynamic_cast<const IR::Type_Method *>(
typeMap ? typeMap->getType(mc->method) : mc->method->type);
if (!type) {
/* FIXME -- can't find the type of the method -- should be a BUG? */
return true; }
auto param = type->parameters->getParameter(ctxt->child_index);
return param->direction == IR::Direction::Out ||
param->direction == IR::Direction::InOut; }
if (ctxt->parent->node->is<IR::ConstructorCallExpression>()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructor arguments should be directionless, since all constructors are evaluated at compile-time.

/* FIXME -- no constructor types? assume all arguments are inout? */
return true; } }
return false;
}
22 changes: 12 additions & 10 deletions midend/Makefile.am
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
# Makefile for mid-ends

midend_SOURCES = \
midend/actionsInlining.h \
midend/actionsInlining.cpp \
midend/inlining.h \
midend/actionsInlining.h \
midend/actionSynthesis.cpp \
midend/actionSynthesis.h \
midend/inlining.cpp \
midend/removeReturns.h \
midend/inlining.h \
midend/local_copyprop.cpp \
midend/local_copyprop.h \
midend/moveConstructors.cpp \
midend/moveConstructors.h \
midend/moveDeclarations.cpp \
midend/moveDeclarations.h \
midend/removeReturns.cpp \
midend/uniqueNames.h \
midend/removeReturns.h \
midend/uniqueNames.cpp \
midend/moveDeclarations.h \
midend/moveDeclarations.cpp \
midend/moveConstructors.h \
midend/moveConstructors.cpp \
midend/actionSynthesis.h \
midend/actionSynthesis.cpp
midend/uniqueNames.h

cpplint_FILES += $(midend_SOURCES)
27 changes: 27 additions & 0 deletions midend/expr_uses.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#ifndef MIDEND_EXPR_USES_H_
#define MIDEND_EXPR_USES_H_

#include "ir/ir.h"

/* Should this be a method on IR::Expression? */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is "no".


class exprUses : public Inspector {
cstring look_for;
bool result = false;
bool preorder(const IR::Path *p) override {
if (p->name == look_for) result = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look right. There could be many identifiers with the same name, but which represent different objects.

Copy link
Contributor Author

@ChrisDodd ChrisDodd May 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in general this only works after identifiers have been made unique -- a major reason to make identifiers unique in the first place. This is a general "does an expression use something named 'name'", with the understanding that if multiple things are named 'name', you'll get any of them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then perhaps this should be documented in some comment.

On Mon, May 2, 2016 at 9:23 AM, Chris Dodd notifications@github.com wrote:

In midend/expr_uses.h
#33 (comment):

@@ -0,0 +1,27 @@
+#ifndef MIDEND_EXPR_USES_H_
+#define MIDEND_EXPR_USES_H_
+
+#include "ir/ir.h"
+
+/* Should this be a method on IR::Expression? */
+
+class exprUses : public Inspector {

  • cstring look_for;
  • bool result = false;
  • bool preorder(const IR::Path *p) override {
  •    if (p->name == look_for) result = true;
    

Yes, in general this only works after identifiers have been made unique --
a major reason to make identifiers unique in the first place.


You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
https://github.com/p4lang/p4c/pull/33/files/2d18de163b8424f9b4a314222fef6b0409f99ada#r61762010

return !result; }
bool preorder(const IR::Primitive *p) override {
if (p->name == look_for) result = true;
return !result; }
bool preorder(const IR::NamedRef *n) override {
if (n->name == look_for) result = true;
return !result; }
bool preorder(const IR::Expression *) override { return !result; }
public:
exprUses(const IR::Expression *e, cstring n) : look_for(n) { e->apply(*this); }
explicit operator bool () { return result; }
};


#endif /* MIDEND_EXPR_USES_H_ */
21 changes: 21 additions & 0 deletions midend/has_side_effects.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#ifndef MIDEND_HAS_SIDE_EFFECTS_H_
#define MIDEND_HAS_SIDE_EFFECTS_H_

#include "ir/ir.h"

/* Should this be a method on IR::Expression? */

class hasSideEffects : public Inspector {
Copy link
Contributor

@mbudiu-bfn mbudiu-bfn May 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a class doing this in frontends/p4/simplify.h - which is also an over-approximation. I should fix this. Unfortunately that class will also need a typeMap to be more precise.

My does not handle primitives - it is just for P4 v1.2. Note that there there are no Primitive nodes in the 1.2 representation.

bool result = false;
bool preorder(const IR::AssignmentStatement *) override { return !(result = true); }
/* FIXME -- currently assuming all calls and primitves have side effects */
bool preorder(const IR::MethodCallExpression *) override { return !(result = true); }
bool preorder(const IR::Primitive *) override { return !(result = true); }
bool preorder(const IR::Expression *) override { return !result; }
public:
explicit hasSideEffects(const IR::Expression *e) { e->apply(*this); }
explicit operator bool () { return result; }
};


#endif /* MIDEND_HAS_SIDE_EFFECTS_H_ */
131 changes: 131 additions & 0 deletions midend/local_copyprop.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
#include "local_copyprop.h"
#include "has_side_effects.h"
#include "expr_uses.h"

class P4::LocalCopyPropagation::ElimDead : public Transform {
LocalCopyPropagation &self;
IR::Declaration_Variable *preorder(IR::Declaration_Variable *var) override {
if (auto local = ::getref(self.locals, var->name)) {
if (!local->live) {
LOG3(" removing dead local " << var->name);
return nullptr; }
} else {
BUG("local %s isn't in locals table", var->name); }
return var; }
IR::AssignmentStatement *postorder(IR::AssignmentStatement *as) override {
if (auto dest = as->left->to<IR::PathExpression>()) {
if (auto local = ::getref(self.locals, dest->path->name)) {
if (!local->live) {
LOG3(" removing dead assignment to " << dest->path->name);
return nullptr; } } }
return as; }

public:
explicit ElimDead(LocalCopyPropagation &self) : self(self) {}
};

void P4::LocalCopyPropagation::flow_merge(Visitor &a_) {
auto &a = dynamic_cast<LocalCopyPropagation &>(a_);
BUG_CHECK(in_action == a.in_action, "inconsitent LocalCopyPropagation state on merge");
for (auto &local : locals) {
if (auto merge = ::getref(a.locals, local.first)) {
if (merge->val != local.second.val)
local.second.val = nullptr;
if (merge->live)
local.second.live = true;
} else {
local.second.val = nullptr; } }
}

void P4::LocalCopyPropagation::dropLocalsUsing(cstring name) {
for (auto &local : locals) {
if (local.first == name) {
LOG4(" dropping " << name << " as it is being assigned to");
local.second.val = nullptr;
} else if (local.second.val && exprUses(local.second.val, name)) {
LOG4(" dropping " << name << " as it is being used");
local.second.val = nullptr; } }
}

P4::LocalCopyPropagation::LocalCopyPropagation(const P4::TypeMap *typeMap) : typeMap(typeMap) {
visitDagOnce = false;
}

IR::Declaration_Variable *P4::LocalCopyPropagation::postorder(IR::Declaration_Variable *var) {
if (!in_action) return var;
if (locals.count(var->name))
BUG("duplicate var declaration for %s", var->name);
auto &local = locals[var->name];
if (var->initializer) {
if (!hasSideEffects(var->initializer)) {
LOG3(" saving init value for " << var->name);
local.val = var->initializer;
} else {
local.live = true; } }
return var;
}

const IR::Expression *P4::LocalCopyPropagation::postorder(IR::PathExpression *path) {
if (auto local = ::getref(locals, path->path->name)) {
Copy link
Contributor

@mbudiu-bfn mbudiu-bfn May 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks wrong to ignore the path prefix. This could be a reference to another variable with this name. You need to use the ReferenceMap to resolve this symbol to know it's true identify. The locals should be a map indexed with an IDeclaration, and not with a cstring.

if (isWrite(typeMap)) {
return path;
} else if (local->val) {
LOG3(" propagating value for " << path->path->name);
return local->val;
} else {
LOG4(" using " << path->path->name << " with no propagated value");
local->live = true; } }
return path;
}

IR::AssignmentStatement *P4::LocalCopyPropagation::postorder(IR::AssignmentStatement *as) {
if (as->left == as->right) { // FIXME -- need deep equals here?
LOG3(" removing noop assignment " << *as);
return nullptr; }
if (auto dest = as->left->to<IR::PathExpression>()) {
dropLocalsUsing(dest->path->name);
if (auto local = ::getref(locals, dest->path->name)) {
if (!hasSideEffects(as->right)) {
LOG3(" saving value for " << dest->path->name);
local->val = as->right; } } }
return as;
}

IR::MethodCallExpression *P4::LocalCopyPropagation::postorder(IR::MethodCallExpression *mc) {
if (!in_action) return mc;
auto type = typeMap->getType(mc->method)->to<IR::Type_Method>();
int idx = 0;
for (auto param : Values(type->parameters->parameters)) {
if (param->direction == IR::Direction::Out || param->direction == IR::Direction::InOut) {
if (auto arg = mc->arguments->at(idx)->to<IR::PathExpression>()) {
dropLocalsUsing(arg->path->name); } }
++idx; }
return mc;
}

IR::Primitive *P4::LocalCopyPropagation::postorder(IR::Primitive *prim) {
if (!in_action) return prim;
for (unsigned idx = 0; idx < prim->operands.size(); ++idx) {
if (prim->isOutput(idx+1)) {
dropLocalsUsing(prim->operands.at(idx)->toString()); } }
return prim;
}

IR::P4Action *P4::LocalCopyPropagation::preorder(IR::P4Action *act) {
in_action = true;
if (!locals.empty()) BUG("corrupt internal data struct");
LOG2("LocalCopyPropagation working on action " << act->name);
LOG4(act);
return act;
}

IR::P4Action *P4::LocalCopyPropagation::postorder(IR::P4Action *act) {
LOG5("LocalCopyPropagation before ElimDead " << act->name);
LOG5(act);
act->body = act->body->apply(ElimDead(*this));
in_action = false;
locals.clear();
LOG3("LocalCopyPropagation finished action " << act->name);
LOG4(act);
return act;
}
Loading