Skip to content

Commit 285594a

Browse files
Calin Cascavalantoninbas
Calin Cascaval
authored andcommitted
Revert "New checksum APIs in v1model; better alias analysis in side-effects; … (#858)"
This reverts commit f09a95d. This commit breaks all existing programs using checksums. This is a change that needs to be staged more carefully, announced on the mailing list and deprecate the old way in a more graceful manner, giving people the chance of transitioning their programs.
1 parent 3318152 commit 285594a

File tree

124 files changed

+1880
-779
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

124 files changed

+1880
-779
lines changed

backends/bmv2/backend.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ Backend::process(const IR::ToplevelBlock* tlb, BMV2Options& options) {
106106
new P4::ConstantFolding(refMap, typeMap, false),
107107
new P4::TypeChecking(refMap, typeMap),
108108
new RemoveComplexExpressions(refMap, typeMap, new ProcessControls(&pipeline_controls)),
109+
new FixupChecksum(&update_checksum_controls),
109110
new P4::SimplifyControlFlow(refMap, typeMap),
110111
new P4::RemoveAllUnusedDeclarations(refMap),
111112
new DiscoverStructure(&structure),

backends/bmv2/lower.cpp

+242
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,248 @@ const IR::Node* LowerExpressions::postorder(IR::Concat* expression) {
136136

137137
namespace {
138138

139+
/**
140+
A list of assignments that all write to a "variable".
141+
This really only handles scalar variables.
142+
It is customized for the needs of FixupChecksum.
143+
*/
144+
struct VariableWriters {
145+
std::set<const IR::AssignmentStatement*> writers;
146+
VariableWriters() = default;
147+
explicit VariableWriters(const IR::AssignmentStatement* writer)
148+
{ writers.emplace(writer); }
149+
VariableWriters* join(const VariableWriters* other) const {
150+
auto result = new VariableWriters();
151+
result->writers = writers;
152+
for (auto e : other->writers)
153+
result->writers.emplace(e);
154+
return result;
155+
}
156+
/**
157+
This function returns a non-null value only if there is exaclty one writer statement.
158+
In that case it returns the RHS of the assignment
159+
*/
160+
const IR::Expression* substitution() const {
161+
if (writers.size() != 1)
162+
return nullptr;
163+
auto first = *writers.begin();
164+
return first->right;
165+
}
166+
};
167+
168+
/**
169+
Maintains a map from variable names to VariableWriters
170+
It is customized for the needs of FixupChecksum.
171+
*/
172+
struct VariableDefinitions {
173+
std::map<cstring, const VariableWriters*> writers;
174+
VariableDefinitions(const VariableDefinitions& other) = default;
175+
VariableDefinitions() = default;
176+
VariableDefinitions* clone() const {
177+
return new VariableDefinitions(*this);
178+
}
179+
VariableDefinitions* join(const VariableDefinitions* other) const {
180+
auto result = clone();
181+
for (auto e : other->writers) {
182+
auto &prev = result->writers[e.first];
183+
prev = prev ? prev->join(e.second) : e.second;
184+
}
185+
return result;
186+
}
187+
void declare(const IR::Declaration_Variable* decl) {
188+
writers.emplace(decl->getName().name, new VariableWriters());
189+
}
190+
const VariableWriters* getWriters(const IR::Path* path) {
191+
if (path->absolute)
192+
return nullptr;
193+
return ::get(writers, path->name.name);
194+
}
195+
VariableDefinitions* setDefinition(const IR::Path* path,
196+
const IR::AssignmentStatement* statement) {
197+
auto w = getWriters(path);
198+
if (w == nullptr)
199+
// Path does not represent a variable
200+
return this;
201+
auto result = clone();
202+
result->writers[path->name.name] = new VariableWriters(statement);
203+
return result;
204+
}
205+
};
206+
207+
/**
208+
Maintain def-use information.
209+
It is customized for the needs of FixupChecksum.
210+
*/
211+
struct PathSubstitutions {
212+
std::map<const IR::PathExpression*, const IR::Expression*> definitions;
213+
std::set<const IR::AssignmentStatement*> haveUses;
214+
PathSubstitutions() = default;
215+
void add(const IR::PathExpression* path, const IR::Expression* expression) {
216+
definitions.emplace(path, expression);
217+
LOG3("Will substitute " << dbp(path) << " with " << expression);
218+
}
219+
const IR::Expression* get(const IR::PathExpression* path) const {
220+
return ::get(definitions, path);
221+
}
222+
void foundUses(const VariableWriters* writers) {
223+
for (auto w : writers->writers)
224+
haveUses.emplace(w);
225+
}
226+
void foundUses(const IR::AssignmentStatement* statement) {
227+
haveUses.emplace(statement);
228+
}
229+
bool hasUses(const IR::AssignmentStatement* statement) const {
230+
return haveUses.find(statement) != haveUses.end();
231+
}
232+
};
233+
234+
/**
235+
See the SimpleCopyProp pass below for the context in which this
236+
analysis is run. We take advantage that some more complex code
237+
patterns have already been eliminated.
238+
*/
239+
class Accesses : public Inspector {
240+
PathSubstitutions* substitutions;
241+
VariableDefinitions* currentDefinitions;
242+
243+
bool notSupported(const IR::Node* node) {
244+
::error("%1%: not supported in checksum update control", node);
245+
return false;
246+
}
247+
248+
public:
249+
explicit Accesses(PathSubstitutions* substitutions): substitutions(substitutions) {
250+
CHECK_NULL(substitutions); setName("Accesses");
251+
currentDefinitions = new VariableDefinitions();
252+
}
253+
254+
bool preorder(const IR::Declaration_Variable* decl) override {
255+
// we assume all variable declarations are at the beginning
256+
currentDefinitions->declare(decl);
257+
return false;
258+
}
259+
260+
// This is only invoked for read expressions
261+
bool preorder(const IR::PathExpression* expression) override {
262+
auto writers = currentDefinitions->getWriters(expression->path);
263+
if (writers != nullptr) {
264+
if (auto s = writers->substitution())
265+
substitutions->add(expression, s);
266+
else
267+
substitutions->foundUses(writers);
268+
}
269+
return false;
270+
}
271+
272+
bool preorder(const IR::AssignmentStatement* statement) override {
273+
visit(statement->right);
274+
if (statement->left->is<IR::PathExpression>()) {
275+
auto pe = statement->left->to<IR::PathExpression>();
276+
currentDefinitions = currentDefinitions->setDefinition(pe->path, statement);
277+
} else {
278+
substitutions->foundUses(statement);
279+
}
280+
return false;
281+
}
282+
283+
bool preorder(const IR::IfStatement* statement) override {
284+
visit(statement->condition);
285+
auto defs = currentDefinitions->clone();
286+
visit(statement->ifTrue);
287+
auto afterTrue = currentDefinitions;
288+
if (statement->ifFalse != nullptr) {
289+
currentDefinitions = defs;
290+
visit(statement->ifFalse);
291+
currentDefinitions = afterTrue->join(currentDefinitions);
292+
} else {
293+
currentDefinitions = defs->join(afterTrue);
294+
}
295+
return false;
296+
}
297+
298+
bool preorder(const IR::SwitchStatement* statement) override
299+
{ return notSupported(statement); }
300+
301+
bool preorder(const IR::P4Action* action) override
302+
{ return notSupported(action); }
303+
304+
bool preorder(const IR::P4Table* table) override
305+
{ return notSupported(table); }
306+
307+
bool preorder(const IR::ReturnStatement* statement) override
308+
{ return notSupported(statement); }
309+
310+
bool preorder(const IR::ExitStatement* statement) override
311+
{ return notSupported(statement); }
312+
};
313+
314+
class Replace : public Transform {
315+
const PathSubstitutions* substitutions;
316+
public:
317+
explicit Replace(const PathSubstitutions* substitutions): substitutions(substitutions) {
318+
CHECK_NULL(substitutions); setName("Replace"); }
319+
320+
const IR::Node* postorder(IR::AssignmentStatement* statement) override {
321+
if (!substitutions->hasUses(getOriginal<IR::AssignmentStatement>()))
322+
return new IR::EmptyStatement();
323+
return statement;
324+
}
325+
326+
const IR::Node* postorder(IR::PathExpression* expression) override {
327+
auto repl = substitutions->get(getOriginal<IR::PathExpression>());
328+
if (repl != nullptr) {
329+
Replace rpl(substitutions);
330+
auto recurse = repl->apply(rpl);
331+
return recurse;
332+
}
333+
return expression;
334+
}
335+
};
336+
337+
/**
338+
This analysis is only executed on the control which performs
339+
checksum update computations.
340+
341+
This is a simpler variant of copy propagation; it just finds
342+
patterns of the form:
343+
tmp = X;
344+
...
345+
out = tmp;
346+
347+
then it substitutes the definition into the use.
348+
The LocalCopyPropagation pass does not do this, because
349+
it won't consider replacing definitions where the RHS has side-effects.
350+
Since the only method call we accept in the checksum update block
351+
is a checksum unit "get" method (this is not checked here, but
352+
in the json code generator), we know that this method has no side-effects,
353+
so we can safely reorder calls to methods.
354+
Also, this is run after eliminating struct and tuple operations,
355+
so we know that all assignments operate on scalar values.
356+
*/
357+
class SimpleCopyProp : public PassManager {
358+
PathSubstitutions substitutions;
359+
public:
360+
SimpleCopyProp() {
361+
setName("SimpleCopyProp");
362+
passes.push_back(new Accesses(&substitutions));
363+
passes.push_back(new Replace(&substitutions));
364+
}
365+
};
366+
367+
} // namespace
368+
369+
const IR::Node* FixupChecksum::preorder(IR::P4Control* control) {
370+
if (updateChecksumBlocks->find(control->name) != updateChecksumBlocks->end()) {
371+
SimpleCopyProp scp;
372+
return control->apply(scp);
373+
}
374+
return control;
375+
}
376+
377+
////////////////////////////////////////////////////////////////////////////////////
378+
379+
namespace {
380+
139381
/**
140382
Detect whether a Select expression is too complicated for BMv2.
141383
Also used to detect complex expressions that are arguments

backends/bmv2/lower.h

+18
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,24 @@ class LowerExpressions : public Transform {
4949
{ prune(); return table; } // don't simplify expressions in table
5050
};
5151

52+
/**
53+
This pass is a hack to work around current BMv2 limitations:
54+
checksum computations must be expressed in a restricted way, since
55+
the JSON code generator uses simple pattern-matching.
56+
57+
The real solution to this problem is to have the BMv2 simulator use a
58+
real extern for computing and verifying checksums. Then this hack
59+
would not be necessary anymore.
60+
*/
61+
class FixupChecksum : public Transform {
62+
const std::set<cstring>* updateChecksumBlocks;
63+
public:
64+
explicit FixupChecksum(const std::set<cstring>* updateChecksumBlocks) :
65+
updateChecksumBlocks(updateChecksumBlocks)
66+
{ setName("FixupChecksum"); }
67+
const IR::Node* preorder(IR::P4Control* control) override;
68+
};
69+
5270
/**
5371
Policy which selects the control blocks where remove
5472
complex expression is applied.

backends/bmv2/simpleSwitch.cpp

+34-31
Original file line numberDiff line numberDiff line change
@@ -100,10 +100,6 @@ SimpleSwitch::convertHashAlgorithm(cstring algorithm) {
100100
result = "random";
101101
else if (algorithm == v1model.algorithm.identity.name)
102102
result = "identity";
103-
else if (algorithm == v1model.algorithm.csum16.name)
104-
result = "csum16";
105-
else if (algorithm == v1model.algorithm.xor16.name)
106-
result = "xor16";
107103
else
108104
::error("%1%: unexpected algorithm", algorithm);
109105
return result;
@@ -251,9 +247,7 @@ SimpleSwitch::convertExternFunctions(Util::JsonArray *result,
251247
static std::set<cstring> supportedHashAlgorithms = {
252248
v1model.algorithm.crc32.name, v1model.algorithm.crc32_custom.name,
253249
v1model.algorithm.crc16.name, v1model.algorithm.crc16_custom.name,
254-
v1model.algorithm.random.name, v1model.algorithm.identity.name,
255-
v1model.algorithm.csum16.name, v1model.algorithm.xor16.name
256-
};
250+
v1model.algorithm.random.name, v1model.algorithm.identity.name };
257251

258252
if (mc->arguments->size() != 5) {
259253
modelError("Expected 5 arguments for %1%", mc);
@@ -606,38 +600,47 @@ SimpleSwitch::generateUpdate(const IR::BlockStatement *block,
606600
auto typeMap = backend->getTypeMap();
607601
auto refMap = backend->getRefMap();
608602
auto conv = backend->getExpressionConverter();
603+
// Currently this is very hacky to target the very limited support
604+
// for checksums in BMv2 This will work much better when BMv2
605+
// offers a checksum extern.
609606
for (auto stat : block->components) {
607+
if (stat->is<IR::IfStatement>()) {
608+
// The way checksums work in Json, they always ignore the condition!
609+
stat = stat->to<IR::IfStatement>()->ifTrue;
610+
}
610611
if (auto blk = stat->to<IR::BlockStatement>()) {
611612
generateUpdate(blk, checksums, calculations);
612613
continue;
613-
} else if (auto mc = stat->to<IR::MethodCallStatement>()) {
614-
auto mi = P4::MethodInstance::resolve(mc->methodCall, refMap, typeMap);
615-
if (auto em = mi->to<P4::ExternFunction>()) {
616-
if (em->method->name.name == v1model.update_checksum.name) {
617-
BUG_CHECK(mi->expr->arguments->size() == 4, "%1%: Expected 4 arguments", mc);
618-
auto cksum = new Util::JsonObject();
619-
auto ei = P4::EnumInstance::resolve(mi->expr->arguments->at(3), typeMap);
620-
if (ei->name != "csum16") {
621-
::error("%1%: the only supported algorithm is csum16",
622-
mi->expr->arguments->at(3));
623-
return;
614+
} else if (auto assign = stat->to<IR::AssignmentStatement>()) {
615+
if (auto mc = assign->right->to<IR::MethodCallExpression>()) {
616+
auto mi = P4::MethodInstance::resolve(mc, refMap, typeMap);
617+
if (auto em = mi->to<P4::ExternMethod>()) {
618+
if (em->method->name.name == v1model.ck16.get.name &&
619+
em->originalExternType->name.name == v1model.ck16.name) {
620+
if (mi->expr->arguments->size() != 1) {
621+
modelError("%1%: Expected 1 argument", assign->right);
622+
return;
623+
}
624+
auto cksum = new Util::JsonObject();
625+
cstring calcName = createCalculation("csum16", mi->expr->arguments->at(0),
626+
calculations, mc);
627+
cksum->emplace("name", refMap->newName("cksum_"));
628+
cksum->emplace("id", nextId("checksums"));
629+
// TODO(jafingerhut) - add line/col here?
630+
auto jleft = conv->convert(assign->left);
631+
cksum->emplace("target", jleft->to<Util::JsonObject>()->get("value"));
632+
cksum->emplace("type", "generic");
633+
cksum->emplace("calculation", calcName);
634+
checksums->append(cksum);
635+
continue;
624636
}
625-
cstring calcName = createCalculation(ei->name, mi->expr->arguments->at(1),
626-
calculations, mc);
627-
cksum->emplace("name", refMap->newName("cksum_"));
628-
cksum->emplace("id", nextId("checksums"));
629-
// TODO(jafingerhut) - add line/col here?
630-
auto jleft = conv->convert(mi->expr->arguments->at(2));
631-
cksum->emplace("target", jleft->to<Util::JsonObject>()->get("value"));
632-
cksum->emplace("type", "generic");
633-
cksum->emplace("calculation", calcName);
634-
checksums->append(cksum);
635-
continue;
636637
}
637638
}
639+
} else if (auto mc = stat->to<IR::MethodCallStatement>()) {
640+
auto mi = P4::MethodInstance::resolve(mc->methodCall, refMap, typeMap, true);
641+
BUG_CHECK(mi && mi->isApply(), "Call of something other than an apply method");
638642
}
639-
::error("%1%: Only calls to %2% allowed", stat, v1model.update_checksum);
640-
return;
643+
BUG("%1%: not handled yet", stat);
641644
}
642645
}
643646

frontends/p4/fromv1.0/converters.cpp

-2
Original file line numberDiff line numberDiff line change
@@ -970,8 +970,6 @@ class FixExtracts final : public Transform {
970970
do much better than this.
971971
*/
972972
class AdjustLengths : public Transform {
973-
public:
974-
AdjustLengths() { setName("AdjustLengths"); }
975973
const IR::Node* postorder(IR::PathExpression* expression) override {
976974
auto anno = findContext<IR::Annotation>();
977975
if (anno == nullptr)

0 commit comments

Comments
 (0)