constant folding streamlining, bugfixes, and Doxygen comments #541

jnfoster · 2017-04-28T17:00:12Z

Do not merge yet!

Submitting this pull request for comments. More documentation and tidying are needed

The current version of constant folding has two bugs:

It does not fold constants in all cases, or check that declaration constants are initialized with genuine constants. For example, on the input program,

const T t = { 32s10, 32s20 };
const S s = { { 32s15, 32s25}, t };
const int<32> x = t.t1;
const int<32> y = s.s1.t2;
const int<32> w = .t.t1;
const T t1 = (T)s.s1;

constant folding (with type information) yields:

const T t = { 32s10, 32s20 };
const S s = { { 32s15, 32s25 }, t };
const int<32> x = 32s10;
const int<32> y = 32s25;
const int<32> w = 32s10;
const T t1 = { 32s15, 32s25 };

Note that the RHS of the declaration of s is not a constant -- it contains the variable t.

The root cause of this behavior is due to the use of auxiliary data structure constants: the pass does not ensure that all expressions that are inserted into this map are genuine constants, yet it assumes that getConstants, which returns arbitrary entries from constants, are always a constant. Hence, the current check for Declaration_Constants (https://github.com/p4lang/p4c/blob/master/frontends/common/constantFolding.cpp#L76) is not correct.

The fix here is to streamline the use of constants so it is used in exactly two places:

It is populated with genuine constants in the IR::Declaration_Constant case
It is read in the IR::PathExpression (aka variable) case

When it transforms the IR, the constant folding pass attempts to patch the typeMap with type information about the newly inserted nodes. However, the types it inserts are not always correct -- e.g., in particular, in the IR::Member case, when header stack sizes are folded, the type map is updated with `origtype, the type of the expression, but the arbitrary-precision integer may require a cast. Hence, subsequent type-checking passes will rely on the information in the typeMap and assume that no cast is required. However, if the typeMap is ever cleared, then the typeChecker will mutate the node, triggering an error.

This error is not triggered by any of our current tests -- in examples like p4_16_samples/stack.p4, the IR::Member is dead code and is eliminated before the typeMap is cleared. Hence, the error never manifests. However, a close variant can trigger the error:

#include <core.p4>
header h { }
control c(out bit<32> x) {
    apply {
        h[4] stack;
        bit<32> sz = stack.size;
        x = sz;
    }
}
control Simpler(out bit<32> x);
package top(Simpler ctr);
top(c()) main;

The solution proposed here is to change the typeMap to const (which streamlines the constant folding pass) and reconstruct it after type checking. In addition, we statically convert the header stacks length to the appropriate type.

mihaibudiu · 2017-04-28T17:09:15Z

I realized that there are more cases to consider.

H[2] h;
H[h.size +1] h1;
H[h1.size + 1] h2;
Etc.

Also:

control c()(bit<32> sz) { H[sz] h; apply {}}

mihaibudiu

I don't see a test case for the failure cases you have described. Please add new test cases in testdata.

Otherwise it looks fine.

mihaibudiu · 2017-04-28T18:29:13Z

frontends/common/constantFolding.h

    bool typesKnown;
    bool warnings;  // if true emit warnings
    // maps expressions and declarations to their constant values
-    std::map<const IR::Node*, const IR::Expression*> constants;
+    std::map<const IR::Declaration_Constant*, const IR::Expression*> constants;


You have to update the comment above.

Also, please update the reference outputs. This can be done by running each failed test with the -f flag - after you made sure that the produced output is the intended one. For example:
./p4/testdata/p4_16_samples/stack.p4.test -f will update the output produced by this test.

cc10512 · 2017-05-05T16:45:03Z

you should rebase on master, not merge master.

jnfoster · 2017-05-05T16:48:05Z

The GitHub web interface triggers these merges...

cc10512 · 2017-05-05T16:49:26Z

That's why I never trusted GUIs ...

sethfowler · 2017-05-05T20:33:10Z

Yeah. I wish the Github web interface would let you "Update branch" using a rebase instead of a merge. It would save a lot of time.

jnfoster · 2017-05-05T20:48:12Z

Fixed comments.
Updated test outputs, which covers bug 1 above.
Added a new test case for bug 2 above.

ChrisDodd · 2017-05-05T21:41:15Z

frontends/common/constantFolding.cpp

        return expr;
-    if (expr->is<IR::BoolLiteral>())
+    }
+    if (expr->is<IR::BoolLiteral>()) {


Seems like this could be just expr->is<IR::Literal>() to get Constant, BoolLiteral and StringLiteral

String literals are not currently folded, so this would change the logic of the function. Leaving it as-is for now.

mihaibudiu · 2017-05-06T00:30:37Z

frontends/common/constantFolding.cpp

    if (expr->is<IR::ListExpression>()) {
        auto list = expr->to<IR::ListExpression>();
        for (auto e : list->components)
-            if (getConstant(e) == nullptr)
+            if (getConstant(e) == nullptr) {


Our style guidelines do not require braces for this case.
Since @ChrisDodd prefers not to squander vertical space, I have avoided using them; they take 1 extra line.

mihaibudiu · 2017-05-06T00:31:37Z

frontends/common/constantFolding.cpp

        if (init->is<IR::Constant>()) {
            auto cst = init->to<IR::Constant>();
            if (d->type->is<IR::Type_Bits>()) {
                if (cst->type->is<IR::Type_InfInt>() ||
                    (cst->type->is<IR::Type_Bits>() &&
-                     !(*d->type->to<IR::Type_Bits>() == *cst->type->to<IR::Type_Bits>())))
+                     !(*d->type->to<IR::Type_Bits>() == *cst->type->to<IR::Type_Bits>()))) 


You should set-up your editor to delete spaces at the end of lines.

mihaibudiu · 2017-05-06T00:32:40Z

frontends/common/constantFolding.cpp

            }
        }
+        if (init != d->initializer) 


So here no braces?

mihaibudiu · 2017-05-06T00:33:44Z

frontends/common/constantFolding.h

    bool typesKnown;
    bool warnings;  // if true emit warnings
-    // maps expressions and declarations to their constant values
-    std::map<const IR::Node*, const IR::Expression*> constants;
+    // maps declaration constants to constant values


This comment could have three slashes

mihaibudiu · 2017-05-06T00:34:32Z

testdata/p4_16_samples_outputs/stack2.p4

@@ -0,0 +1,16 @@
+#include <core.p4>


How about some of the tests I suggested in comments, e.g.

H[h1.size+1] h2

In principle, this should be possible, but it would be a deeper change based on how the passes are currently organized:

We don't fold IR::Members until after type checking.

In particular, we need to know the type of the expression to create a constant with the appropriate width.

But the type checker requires header stacks to have a compile-time known constant as their size.

This should be either a positive or a negative test. If it is negative it should have a good error message.

Added stack3.p4 as an error test.

jnfoster · 2017-05-07T23:12:45Z

Okay, I think this is finally ready to go. Please let me know if you have more feedback.

ChrisDodd · 2017-05-08T22:22:09Z

frontends/common/constantFolding.cpp

-        return e;
-    return v;
+    if (decl->is<IR::Declaration_Constant>()) {
+        auto dc = decl->to<IR::Declaration_Constant>();


Stylisticly I prefer combining these two lines as

if (auto dc = decl->to<IR::Declaration_Constant>()) {

Actually, this doesn't compile. So I reverted.

You forgot the = sign in your code. It would compile.

Mea culpa! I'll push one more time.

ChrisDodd · 2017-05-08T22:28:38Z

frontends/common/constantFolding.cpp

-        d = new IR::Declaration_Constant(d->srcInfo, d->name, d->annotations, d->type, init);
+    auto orig = getOriginal()->to<IR::Declaration_Constant>();
+    BUG_CHECK(orig, "getOriginal() did not return a Declaration_Constant");
+    constants.emplace(orig, init);


These 3 lines could be replaced by

constants.emplace(getOriginal<IR::Declaration_Constant>(), init);

-- the templated version of getOriginal does the cast and the BUG_CHECK.

Cool. Done.

mihaibudiu · 2017-05-08T22:44:24Z

testdata/p4_16_errors/stack3.p4

@@ -0,0 +1,42 @@
+/*


Where is the output generated by the compiler for this test?
There should be a file stack3.p4-stderr at the very least.

jnfoster · 2017-05-08T22:45:35Z

My fault, I didn't understand the protocol for adding tests that fail to compile. I'll grab the stderr and stash it in the repo now.

…

-N

On Mon, May 8, 2017 at 3:44 PM, Mihai Budiu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In testdata/p4_16_errors/stack3.p4 <#541 (comment)>: > @@ -0,0 +1,42 @@ +/* Where is the output generated by the compiler for this test? There should be a file stack3.p4-stderr at the very least. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#541 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwi0gdvCzpBDLl1aJqcznyQ5TNpK0Prks5r35rIgaJpZM4NLvDc> .

mihaibudiu · 2017-05-08T22:47:13Z

In general you should git add everything in testdata after running a test.

jnfoster · 2017-05-08T22:48:25Z

Got it. Thanks!

…

On Mon, May 8, 2017 at 3:47 PM, Mihai Budiu ***@***.***> wrote: In general you should git add everything in testdata after running a test. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#541 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwi0kxI6GmnZP0ADKumcBXaTqHHT8dfks5r35tygaJpZM4NLvDc> .

mihaibudiu

This commit only contains some changes in the control-plane folder, but no files in testdata.

antoninbas · 2017-05-08T22:53:19Z

@jnfoster I doubt you meant to update the PI submodule ref pointer (you probably shouldn't)

jnfoster · 2017-05-08T22:54:20Z

Gah. I think this is from pushing the update branch button here and then pushing additional changes from the command line. Let me fix this...

jnfoster · 2017-05-08T23:27:16Z

Okay @sethfowler helped me detangle myself.

Morale: don't ever push GitHub's "Update branch" button or follow their command-line instructions!

jnfoster · 2017-05-08T23:36:00Z

testdata/p4_16_errors_outputs/stack3.p4-stderr

+../testdata/p4_16_errors/stack3.p4(35): error: header h[]: Size of header stack type should be a constant
+        h[x] s3;
+        ^^^^
+../testdata/p4_16_errors/stack3.p4(35): error: Could not find type of h[]


@mbudiu-vmw I'm confused about why the type of h can't be found. Is this indicating a deeper error?

No, if a sub-expression fails type-checking no type is installed in the typeMap, so all the expressions that involve it as a sub-expression will fail to type-check too. These additional error messages are somewhat redundant, but other compilers are equally confused.

mihaibudiu · 2017-05-10T15:45:15Z

@jnfoster : this code does not build.

jnfoster · 2017-05-10T16:30:13Z

Sorry about that. Clearly I had not run make check when I committed @ChrisDodd's suggested changes in. I just pushed a new version that walks it back and verified that it compiles and passes all tests on my machine. Should be fixed now.

* Simplify handling of `constants` map. It is now populated in the visitor for `IR::Declaration_Cosntant` nodes and accessed by the visitor for `IR::PathExpression` nodes. * Make `typeMap` `const`. * Polish and streamline the code.

* Add a new failing test (stack3.p4) * Update test outputs for other tests

mihaibudiu requested changes Apr 28, 2017

View reviewed changes

ChrisDodd reviewed May 5, 2017

View reviewed changes

mihaibudiu approved these changes May 6, 2017

View reviewed changes

mihaibudiu requested changes May 6, 2017

View reviewed changes

jnfoster mentioned this pull request May 7, 2017

Polish constant folding #525

Closed

jnfoster changed the title ~~[do not merge] constant folding streamlining and bugfixes~~ constant folding streamlining, bugfixes, and Doxygen comments May 7, 2017

ChrisDodd approved these changes May 8, 2017

View reviewed changes

mihaibudiu reviewed May 8, 2017

View reviewed changes

jnfoster commented May 8, 2017

View reviewed changes

mihaibudiu approved these changes May 8, 2017

View reviewed changes

jnfoster added 3 commits May 10, 2017 12:02

Refactor ConstantFolding pass

c0eacbf

* Simplify handling of `constants` map. It is now populated in the visitor for `IR::Declaration_Cosntant` nodes and accessed by the visitor for `IR::PathExpression` nodes. * Make `typeMap` `const`. * Polish and streamline the code.

Add new tests involving constant folding for header stacks

fc11c0f

* Add a new failing test (stack3.p4) * Update test outputs for other tests

Add Doxygen comments for ConstantFolding pass

0935389

jnfoster merged commit 07f93f8 into p4lang:master May 10, 2017

fefer mentioned this pull request Aug 7, 2018

"make" get's 100% without error, but "make check" fails #1433

Closed

constant folding streamlining, bugfixes, and Doxygen comments #541

constant folding streamlining, bugfixes, and Doxygen comments #541

Conversation

jnfoster commented Apr 28, 2017 • edited Loading

mihaibudiu commented Apr 28, 2017

mihaibudiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cc10512 commented May 5, 2017

jnfoster commented May 5, 2017

cc10512 commented May 5, 2017

sethfowler commented May 5, 2017

jnfoster commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnfoster commented May 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnfoster commented May 8, 2017 via email

mihaibudiu commented May 8, 2017

jnfoster commented May 8, 2017 via email

mihaibudiu left a comment

Choose a reason for hiding this comment

antoninbas commented May 8, 2017

jnfoster commented May 8, 2017

jnfoster commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu commented May 10, 2017

jnfoster commented May 10, 2017

jnfoster commented Apr 28, 2017 •

edited

Loading