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.

Sign up for GitHub

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

Jump to bottom

Shorten the Identifer name, including dots(.) in Member expression #3175

Merged
merged 1 commit into from
Apr 8, 2022
Merged

Shorten the Identifer name, including dots(.) in Member expression #3175

merged 1 commit into from
Apr 8, 2022

Conversation

kamleshbhalui
Copy link
Contributor

to be less than 64 because dpdk does not allow Identifer longer than 63 chars

mihaibudiu
mihaibudiu requested changes Apr 5, 2022
View reviewed changes
backends/dpdk/dpdk.def Outdated
@@ -1,38 +1,38 @@
interface IDPDKNode {
virtual std::ostream& toSpec(std::ostream& out) const = 0;
virtual std::ostream& toSpec(std::ostream& out, const ordered_map<cstring, cstring>& origNameMap = {}) const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can make the map accessible through a method of the IDPDKNode class. Could be even a static variable (not beautiful, but simpler).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

create static member

backends/dpdk/dpdkAsmOpt.h
ordered_map<cstring, cstring> newNameMap;
ordered_map<cstring, cstring> &origNameMap;
static size_t count;
// Currently Dpdk allows Identifier of 63 char long or less
Copy link
Contributor

Choose a reason for hiding this comment

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

this is just a silly restriction, since I suspect the identifiers do not have a runtime representation.
wouldn't it be easier to fix this in the DPDK spec/implementation?
we're no longer in the 1970s when every byte counted.

backends/dpdk/dpdkAsmOpt.h
// including dots(.) for member exp.
// worst case member expression will look like below(for headers)
// 1.30.30 => 63(including dot(.))
cstring shortenString(cstring str, size_t allowedLength = 60) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a class which does this, MinimalNameGenerator.
You should reuse that code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This runs post codegen and applied on some dpdk specific types too.

@kamleshbhalui kamleshbhalui force-pushed the large_name branch 2 times, most recently from e03d7d6 to 9499681 Compare April 6, 2022 13:46
mihaibudiu
mihaibudiu requested changes Apr 6, 2022
View reviewed changes
backends/dpdk/spec.cpp Outdated
@@ -16,22 +17,39 @@ std::ostream &IR::DpdkAsmProgram::toSpec(std::ostream &out) const {
l->toSpec(out) << std::endl;
}
out << std::endl;
for (auto h : headerType)
for (auto h : headerType) {
if (origNameMap.count(h->name.toString()))
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this could have been also a utility function, but it's a short one.

ir/ir.cpp Outdated
@@ -34,6 +34,8 @@ const cstring Type_Error::error = "error";

int IR::Declaration::nextId = 0;
int IR::This::nextId = 0;
// used in dpdk backend
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to a separate file, dpdk.cpp.

ir/ir.def Outdated
@@ -613,4 +613,10 @@ class ToplevelBlock : Block, IDeclaration {
validate { BUG_CHECK(node->is<IR::P4Program>(), "%1%: expected a P4Program", node); }
}

/// Dpdk specific ir node
interface IDPDKNode {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in a separate file, dpdk.def.

@kamleshbhalui kamleshbhalui force-pushed the large_name branch 4 times, most recently from 5ed1815 to a87ac30 Compare April 7, 2022 15:44
mihaibudiu
mihaibudiu approved these changes Apr 7, 2022
View reviewed changes
BUILD.bazel
@@ -275,8 +280,71 @@ cc_binary(
data = [":p4include"],
)

# This builds the p4test backend.
# dpdk backend
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps for the bazel build you want to request a review from @smolkaj

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am unable to add @smolkaj to reviewer list. may be due to permission issues. Can you please add him?

Copy link
Contributor

Choose a reason for hiding this comment

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

That mention is enough, he had been notified. If he doesn't reply in a couple of days I will merge it anyway

Copy link
Member

Choose a reason for hiding this comment

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

Got it, taking a look :)

hanw
hanw reviewed Apr 8, 2022
View reviewed changes
backends/dpdk/dpdk.def Outdated
@@ -1,5 +1,6 @@
interface IDPDKNode {
virtual std::ostream& toSpec(std::ostream& out) const = 0;
static ordered_map<cstring, cstring> origNameMap;
Copy link
Contributor

@hanw hanw Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

How about change this to be a static memeber of the ShortenTokenLength class? This feels like an odd place for a static member.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@kamleshbhalui
[dpdk] Shorten the Identifer name, including dots(.) in Member expres…
3be4e04 
…sion

to be less than 64 because dpdk does not allow Identifer longer than 63 chars
smolkaj
smolkaj approved these changes Apr 8, 2022
View reviewed changes
Copy link
Member

@smolkaj smolkaj left a comment

Choose a reason for hiding this comment

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

LGTM for the BUILD file changes

BUILD.bazel
cc_library(
name = "p4c_dpdk_lib",
srcs = glob(
["backends/dpdk/*.cpp", "backends/dpdk/control-plane/*.cpp", "backends/bmv2/common/lower.cpp"],
Copy link
Member

Choose a reason for hiding this comment

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

Nit: maybe put each file on a separate line for consistency / readability?

Copy link
Member

Choose a reason for hiding this comment

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

Here and below

@mihaibudiu
Copy link
Contributor

I will merge this now, but please consider the suggested changes in a future PR. Thank you all.

@mihaibudiu mihaibudiu merged commit 13c396d into p4lang:main Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihaibudiu mihaibudiu mihaibudiu approved these changes

@hanw hanw hanw left review comments

@smolkaj smolkaj smolkaj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kamleshbhalui @mihaibudiu @hanw @smolkaj