Skip to content

[Generator] Add an explicit modifier to the IR generator. #5208

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
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
2 changes: 2 additions & 0 deletions .github/workflows/auto-label.yml
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ jobs:
"[frontend]":"core",
"[midend]":"core",
"[parser]":"core",
"[generator]":"core",
};
} else {
// PR label mappings (only the ones you want for PRs)
Expand All @@ -74,6 +75,7 @@ jobs:
"[infra]": "infrastructure",
"[midend]":"core",
"[parser]":"core",
"[generator]":"core",
"[dpdk]": "dpdk",
"[p4-spec]": "p4-spec",
"[tofino]":"tofino",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
#include <tna.p4>

header foo_t {
varbit<32> foo;
}
struct headers {
foo_t foo;
}
struct ingress_metadata_t {}
struct egress_headers_t {}
struct egress_metadata_t {}


parser ig_prs(
packet_in pkt,
out headers hdr,
out ingress_metadata_t ig_md,
out ingress_intrinsic_metadata_t ig_intr_md)
{
state start {
transition foo;
}

state foo {
pkt.extract(hdr.foo, (bit<32>)8);
transition accept;
}
}

control ig_ctl(
inout headers hdr, inout ingress_metadata_t ig_md,
in ingress_intrinsic_metadata_t ig_intr_md,
in ingress_intrinsic_metadata_from_parser_t ig_prsr_md,
inout ingress_intrinsic_metadata_for_deparser_t ig_dprsr_md,
inout ingress_intrinsic_metadata_for_tm_t ig_tm_md)
{
apply {
}
}

control ig_dprs(
packet_out pkt,
inout headers hdr,
in ingress_metadata_t ig_md,
in ingress_intrinsic_metadata_for_deparser_t ig_dprsr_md)
{
apply {
}
}

parser eg_prs(
packet_in pkt,
out egress_headers_t eg_hdr, out egress_metadata_t eg_md,
out egress_intrinsic_metadata_t eg_intr_md)
{
state start {
transition accept;
}
}

control eg_ctl(
inout egress_headers_t eg_hdr, inout egress_metadata_t eg_md,
in egress_intrinsic_metadata_t eg_intr_md,
in egress_intrinsic_metadata_from_parser_t eg_prsr_md,
inout egress_intrinsic_metadata_for_deparser_t eg_dprsr_md,
inout egress_intrinsic_metadata_for_output_port_t eg_oport_md)
{
apply {
}
}

control eg_dprs(
packet_out pkt,
inout egress_headers_t eg_hdr, in egress_metadata_t eg_md,
in egress_intrinsic_metadata_for_deparser_t eg_dprsr_md)
{
apply {
}
}

Pipeline(
ig_prs(), ig_ctl(), ig_dprs(),
eg_prs(), eg_ctl(), eg_dprs()) pipe;

Switch(pipe) main;
2 changes: 1 addition & 1 deletion backends/tofino/bf-p4c/midend/desugar_varbit_extract.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class AnnotateVarbitExtractStates : public Transform {
if (!method) continue;

if (method->member == "extract" && call->arguments->size() == 2) {
state->addOrReplaceAnnotation("dontmerge"_cs, {});
state->addOrReplaceAnnotation(new IR::Annotation("dontmerge"_cs, {}));
break;
}
}
Expand Down
17 changes: 13 additions & 4 deletions backends/tofino/bf-p4c/parde/extract_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -470,11 +470,20 @@ const IR::BFN::Parser *GetBackendParser::createBackendParser() {
backendState->stride = true;
LOG3("mark " << state->name << " as strided");
}
if (auto dontmerge = state->getAnnotation("dontmerge"_cs)) {
if (dontmerge->getExpr().size()) {
auto gress = dontmerge->getExpr(0)->to<IR::StringLiteral>();
if (gress->value == toString(parser->thread)) {
for (const auto *annotation : state->getAnnotations()) {
if (annotation->name == "dontmerge") {
// If the annotation has a thread or "gress" argument, check whether the gress
// matches this parser's thread and only then set dontMerge to true.
// Otherwise, don't merge.
if (annotation->getExpr().size() != 0U) {
const auto *gress = annotation->getExpr(0)->to<IR::StringLiteral>();
if (gress->value == toString(parser->thread)) {
backendState->dontMerge = true;
break;
}
} else {
backendState->dontMerge = true;
break;
}
}
}
Expand Down
10 changes: 8 additions & 2 deletions ir/base.def
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ class Annotation {
inline Annotation(Util::SourceInfo si, ID n,
std::initializer_list<const Expression *> a, bool structured = false)
: Node(si), name(n), body(a), structured(structured) {}
inline Annotation(Util::SourceInfo si, ID n,
inline explicit Annotation(Util::SourceInfo si, ID n,
const Expression *a, bool structured = false)
: Node(si), name(n), body(), structured(structured) {
body.emplace<ExpressionAnnotation>(a);
Expand All @@ -252,7 +252,7 @@ class Annotation {
inline Annotation(Util::SourceInfo si, ID n, const IndexedVector<NamedExpression> &kv,
bool structured = false)
: Node(si), name(n), body(kv), structured(structured) {}
inline Annotation(ID n, const Expression *a, bool structured = false)
inline explicit Annotation(ID n, const Expression *a, bool structured = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem like the explicit does anything useful here -- it just disallows things like

IR::Annotation annot = { some_id, some_expression };

which doesn't seem too useful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, I started out with this approach and then realized the Expression constructor must be made explicit. Figured it is still useful to add this modifier to the generator.

I haven't made the necessary changes to the generator yet to support this. There is still a bit of work to do.

: name(n), body(), structured(structured) {
body.emplace<ExpressionAnnotation>(a);
}
Expand Down Expand Up @@ -445,6 +445,12 @@ interface IAnnotated {
inline void addOrReplaceAnnotation(Annotation ann) {
Annotations::addOrReplace(getAnnotations(), ann);
}
#emit
// FIXME: We shouldn't need this delete.
// Ideally, we only accept references to expressions.
void addOrReplaceAnnotation(cstring name, std::nullptr_t expr, bool structured = false) = delete;
void addAnnotation(cstring name, std::nullptr_t expr, bool structured = false) = delete;
Copy link
Contributor

Choose a reason for hiding this comment

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

While calling these with an explicit nullptr doesn't make sense, does this do anything for the {} case? I think what we need is something like:

inline void addOrReplaceAnnotation(cstring name, std::initializer_list<const Expression *> elist, bool structured = false) {
    Annotations::addOrReplace(getAnnotations(), new Annotation(name, elist, structured));
}

or perhaps add Annotations::addOrReplace(Vector<Annotation> &, cstring, std::initializer_list<const Expression *>, bool) and call that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleting this overload works because the compiler will throw an error that the call is ambiguous. It is not pretty, but doesn't break the interface.

Probably the right approach is to make everything pass-by-reference.

#end
}

interface IInstance {
Expand Down
1 change: 1 addition & 0 deletions tools/ir-generator/ir-generator-lex.l
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ static std::string comment_block;
"delete" { return DELETE; }
"enum" { return ENUM; }
"inline" { return INLINE; }
"explicit" { return EXPLICIT; }
"interface" { return INTERFACE; }
"namespace" { return NAMESPACE; }
"new" { return NEW; }
Expand Down
12 changes: 8 additions & 4 deletions tools/ir-generator/ir-generator.ypp
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,9 @@ static IrNamespace *current_namespace = LookupScope().resolve(0);
} emit;
}

%token ABSTRACT APPLY CLASS CONST DBLCOL DEFAULT DELETE ENUM INLINE INTERFACE NAMESPACE
NEW NULLOK OPERATOR OPTIONAL PRIVATE PROTECTED PUBLIC STATIC VARIANT VIRTUAL
%token ABSTRACT APPLY CLASS CONST DBLCOL DEFAULT DELETE ENUM INLINE EXPLICIT
INTERFACE NAMESPACE NEW NULLOK OPERATOR OPTIONAL PRIVATE PROTECTED
PUBLIC STATIC VARIANT VIRTUAL
%token<str> BLOCK COMMENTBLOCK IDENTIFIER INCLUDE INTEGER NO STRING ZERO
%token<emit> EMITBLOCK

Expand Down Expand Up @@ -255,9 +256,11 @@ method
{ if ($2 != $<irClass>0->name)
yyerror("constructor name %s doesn't match class name %s", $2.c_str(),
$<irClass>0->name.c_str());
if ($1 & ~IrField::Inline)
if ($1 & ~(IrField::Inline | IrField::Explicit))
yyerror("%s invalid on constructor", IrElement::modifier($1));
($<irMethod>$ = new IrMethod(@2, $2))->inImpl = !($1 & IrField::Inline);
$<irMethod>$ = new IrMethod(@2, $2);
$<irMethod>$->inImpl = !($1 & IrField::Inline);
$<irMethod>$->isExplicit = ($1 & IrField::Explicit);
$<irMethod>$->clss = $<irClass>0; }
optArgList { BEGIN(PARSE_CTOR_INIT); } ')' body
{ ($$ = $<irMethod>4)->body = $8;
Expand Down Expand Up @@ -341,6 +344,7 @@ irField
modifier
: NULLOK { $$ = IrElement::NullOK; }
| INLINE { $$ = IrElement::Inline; }
| EXPLICIT { $$ = IrElement::Explicit; }
| OPTIONAL { $$ = IrElement::Optional; }
| STATIC { $$ = IrElement::Static; }
| VIRTUAL { $$ = IrElement::Virtual; }
Expand Down
1 change: 1 addition & 0 deletions tools/ir-generator/irclass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ void IrMethod::generate_hdr(std::ostream &out) const {
if (isStatic) out << "static ";
if (isVirtual) out << "virtual ";
if (isFriend) out << "friend ";
if (isExplicit) out << "explicit ";
generate_proto(out, false, isUser);
if (isOverride) out << " override";
if (inImpl || !body)
Expand Down
4 changes: 3 additions & 1 deletion tools/ir-generator/irclass.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class IrElement : public Util::IHasSourceInfo {
Virtual = 1 << 3,
Static = 1 << 4,
Const = 1 << 5,
Explicit = 1 << 6,
};
static inline const char *modifier(int m) {
if (m & IrElement::NullOK) return "NullOK";
Expand All @@ -104,6 +105,7 @@ class IrElement : public Util::IHasSourceInfo {
if (m & IrElement::Static) return "static";
if (m & IrElement::Inline) return "inline";
if (m & IrElement::Const) return "const";
if (m & IrElement::Explicit) return "explicit";
return "";
}
};
Expand All @@ -129,7 +131,7 @@ class IrMethod : public IrElement {
std::vector<const IrField *> args;
cstring body;
bool inImpl = false, isConst = false, isOverride = false, isStatic = false, isVirtual = false,
isUser = false, isFriend = false;
isUser = false, isFriend = false, isExplicit = false;
IrMethod(Util::SourceInfo info, cstring name, cstring body)
: IrElement(info), name(name), body(body) {}
IrMethod(Util::SourceInfo info, cstring name) : IrElement(info), name(name) {}
Expand Down
Loading