Syntax sugar for creating IR::Expressions #196

ChrisDodd · 2016-12-20T16:45:29Z

IR::ERef type encapsulating a const IR::Expression * for overloading
overloaded operators and methods for creating new expressions
set types & srcInfo of newly created expressiong based on operands

- IR::ERef type encapsulating a `const IR::Expression *` for overloading - overloaded operators and methods for creating new expressions - set types & srcInfo of newly created expressions based on operands

mihaibudiu

While this looks pretty it has two major shortcomings:

you are losing sourceInfo for many constructs. This will cause problems with giving good error messages and implementing debuggers
you are mixing some type checking logic with the IR classes. I think that typeChecking should be a visitor.

mihaibudiu · 2016-12-20T16:49:08Z

frontends/p4-14/p4-14-parse.ypp

-    | '!' expression %prec PREFIX       { $$ = new IR::LNot(@1, $2); }
-    | '~' expression %prec PREFIX       { $$ = new IR::Cmpl(@1, $2); }
-    | '-' expression %prec PREFIX       { $$ = new IR::Neg(@1, $2); }
+    | NOT expression %prec PREFIX       { $$ = !$2; }


The grammar was partly serving as documentation of the IR; now developers will have to read the ERef class too.

You are losing the sourceInfo. Error messages will not be able to point to the source code.

source info is pulled from the operands by default -- it should be unchanged from what it was before (all tests indicate that it is unchanged)

OK, then I withdraw this objection.
Should have read more carefully ERef.

Actually, sourceInfo was pointing at the operation sign only, but maybe that's a minor issue.
The error looks like this:

a+b ^

and not like this:

a+b ^^^

This is an odd difference between the P4_14 and _16 frontends that I hadn't noticed before -- the P4_14 frontend always set the source info to the full expression (not just the operator token). I'm not sure which is better aesthetically for error messages. We could change it back to explicity setting the source info to just the operator char if desired.

P4-16 was the same originally, but this causes problems with long expressions that span multiple lines; the error output does not look as sharp - so I moved to pointing just the operator instead of also pointing the operands.

mihaibudiu · 2016-12-20T16:52:14Z

frontends/p4/fromv1.0/converters.cpp

-                                    new IR::Constant(aval));
-        result->type = IR::Type_Bits::get(bval);
+        IR::ERef lookahead = method(typeargs);
+        IR::ERef result = lookahead.Slice(aval + bval - 1, aval);


You are losing the source info in the slice.

source info is unchanged here as far as I can see...

mihaibudiu · 2016-12-20T16:52:39Z

frontends/p4/fromv1.0/converters.cpp

-            new IR::Vector<IR::Expression>());
+        IR::ERef base = primitive->operands.at(0);
+        IR::ERef method = base.Member(primitive->srcInfo, IR::Type_Header::isValid);
+        IR::ERef result = method();


You are losing the sourceInfo in the method.

unchanged here as well

mihaibudiu · 2016-12-20T16:53:02Z

frontends/p4/fromv1.0/converters.cpp

-                                             structure->emptyTypeArguments,
-                                             new IR::Vector<IR::Expression>());
+    IR::ERef tbl = new IR::PathExpression(newname);
+    IR::ERef method = tbl.Member(IR::IApply::applyMethodName);


All these are losing the sourceInfo.

mihaibudiu · 2016-12-20T16:56:54Z

frontends/p4/p4-parse.ypp

@@ -452,8 +452,8 @@ simpleExpressionList

 simpleKeysetExpression
    : expression                  { $$ = $1; }
-    | expression MASK expression  { $$ = new IR::Mask(@1 + @3, $1, $3); }
-    | expression RANGE expression { $$ = new IR::Range(@1 + @3, $1, $3); }
+    | expression MASK expression  { $$ = $1.Mask($3); }


These are losing the sourceInfo.

mihaibudiu · 2016-12-20T16:57:49Z

frontends/p4/p4-parse.ypp

-    : lvalue '(' argumentList ')' ';'  { auto mc = new IR::MethodCallExpression(@1 + @4, $1,
-                                                               new IR::Vector<IR::Type>(), $3);
-                                         $$ = new IR::MethodCallStatement(@1 + @4, mc); }
+    : lvalue '(' argumentList ')' ';'  { $$ = new IR::MethodCallStatement(@1 + @4, $1($3)); }


Same here for the methodCallExpression.

mihaibudiu · 2016-12-20T16:59:15Z

ir/expression.def

@@ -66,10 +66,12 @@ class Sub : Operation_Binary {
 class Shl : Operation_Binary {
    stringOp = "<<";
    precedence = DBPrint::Prec_Shl;
+    Shl { if (type->is<Type::Unknown>() && left) type = left->type; }


I don't think that this logic belongs to the IR class; it is maybe part of a typeChecker.

We want the types set for expressions in all cases, so it should be part of the constructor. Making it a separate pass just leads to holes where the type info is incorrect.

The P4-16 front-end and all mid-end passes I wrote never use the type in the expression; they always uses the typeMap. The only use for that type is when converting from p4-14 - the type is set by the p4-14 front-end and read by the p4-16 front-end.

ChrisDodd · 2016-12-20T17:30:36Z

I would dearly love to get rid of the typeMap and refMap and just use the types in the IR everywhere, as they are a huge source of excess complexity and bugs in the code. They are needed at first to do type resolution in the frontend, but they should no longer be necessary in the midend. Fixing this would require adding all declarations to the global scope (once they've been changed to be globally unique), after which any code that needs to get the definition for a name can just look in the global scope. This would interfere with dumping the code as legal P4, however (perhaps just mark these added symbols and have the toP4 code ignore them in the global scope?) -chris

…

On Tue, Dec 20, 2016 at 9:23 AM, Mihai Budiu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ir/expression.def <#196>: > @@ -66,10 +66,12 @@ class Sub : Operation_Binary { class Shl : Operation_Binary { stringOp = "<<"; precedence = DBPrint::Prec_Shl; + Shl { if (type->is<Type::Unknown>() && left) type = left->type; } The P4-16 front-end and all mid-end passes I wrote never use the type in the expression; they always uses the typeMap. The only use for that type is when converting from p4-14 - the type is set by the p4-14 front-end and read by the p4-16 front-end. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#196>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD4c8WSW7iaeD0ILi_GADlODP5tTHDS0ks5rKA8vgaJpZM4LSC78> .

mihaibudiu · 2016-12-20T18:15:09Z

Changing all existing passes will be a major endeavor. The simplest solution is to write a pass before the back-end which does the conversion you want and consume this lower-level IR in the back-end. Also, in the back-end you don't worry about generating P4 again.

Note that types do change while compiling, for example when enums are converted to ints, or when parameters are removed (then function signatures change). So the typechecker is not a no-op in the mid-end.

sethfowler · 2016-12-20T18:55:26Z

ir/expression_ref.h

+
+ public:
+    ERef(const Expression *e) : expr(e) {}   // NOLINT(runtime/explicit)
+    ERef(const Expression &e) : expr(&e) {}   // NOLINT(runtime/explicit)


Are Expression objects always allocated via new? If you pass a temporary here you'll be storing a pointer to the stack. That's a pretty big footgun. Presumably if they are always allocated via new, the constructor that takes a pointer is sufficient, so I'd suggest only providing that one or else performing an actual copy here.

Yes, they should always be allocated by new. This was an attempt to get around the need for explicit IR::ERef constructor calls all over the place, but it doesn't really work.

sethfowler · 2016-12-20T19:06:15Z

ir/expression_ref.h

+
+namespace IR {
+
+class ERef {


I'd mark this class final.

I was thinking that I wanted ERef_specific to be a subclass of ERef to avoid erasing the Expression subclass type, but that doesn't really work either.

ChrisDodd · 2016-12-20T19:56:20Z

I think I'm going to withdraw this pull request as being only half-baked. Experimenting with using ERef in the backend, it seems far too cumbersome, as all the Expression references in the IR are const Expression * rather than ERef, so you end up needing many explicit IR::ERef calls, making the result no more readable than all the explicit new calls. Avoiding that will likely require integrating this with the IR generator to use some sort of explicit reference type in the generated IR classes rather than raw pointers.

ChrisDodd assigned sethfowler and mihaibudiu Dec 20, 2016

ChrisDodd force-pushed the cdodd branch from e581256 to 7d736d4 Compare December 20, 2016 17:00

Syntax sugar for creating IR::Expressions

ed0e282

- IR::ERef type encapsulating a `const IR::Expression *` for overloading - overloaded operators and methods for creating new expressions - set types & srcInfo of newly created expressions based on operands

ChrisDodd force-pushed the cdodd branch from 7d736d4 to ed0e282 Compare December 20, 2016 17:01

mihaibudiu requested changes Dec 20, 2016

View reviewed changes

sethfowler reviewed Dec 20, 2016

View reviewed changes

ChrisDodd closed this Dec 20, 2016

fefer mentioned this pull request Aug 7, 2018

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Syntax sugar for creating IR::Expressions #196

Syntax sugar for creating IR::Expressions #196

ChrisDodd commented Dec 20, 2016

mihaibudiu left a comment

mihaibudiu Dec 20, 2016

ChrisDodd Dec 20, 2016

mihaibudiu Dec 20, 2016

mihaibudiu Dec 20, 2016

ChrisDodd Dec 20, 2016

mihaibudiu Dec 20, 2016

mihaibudiu Dec 20, 2016

ChrisDodd Dec 20, 2016

mihaibudiu Dec 20, 2016

ChrisDodd Dec 20, 2016

mihaibudiu Dec 20, 2016

mihaibudiu Dec 20, 2016

mihaibudiu Dec 20, 2016

mihaibudiu Dec 20, 2016

ChrisDodd Dec 20, 2016

mihaibudiu Dec 20, 2016

ChrisDodd commented Dec 20, 2016 via email

mihaibudiu commented Dec 20, 2016

sethfowler Dec 20, 2016

ChrisDodd Dec 20, 2016

sethfowler Dec 20, 2016

ChrisDodd Dec 20, 2016

ChrisDodd commented Dec 20, 2016

Syntax sugar for creating IR::Expressions #196

Syntax sugar for creating IR::Expressions #196

Conversation

ChrisDodd commented Dec 20, 2016

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

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

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

ChrisDodd commented Dec 20, 2016 via email

mihaibudiu commented Dec 20, 2016

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

ChrisDodd commented Dec 20, 2016