Skip to content

Emit p4info , bfrt and context for MatchValueLookupTable #16

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
Open
kamleshbhalui wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Emit p4info , bfrt and context for MatchValueLookupTable #16

kamleshbhalui wants to merge 11 commits into from

Conversation

@kamleshbhalui
Copy link
Owner

@kamleshbhalui kamleshbhalui commented Jun 1, 2022

Co-authored-by: Sivanupandi, Pitchumani pitchumani.sivanupandi@intel.com

@kamleshbhalui
Copy link
Owner Author

kamleshbhalui commented Jun 1, 2022

correponding changes in kamleshbhalui/p4runtime#1

usha1830
usha1830 reviewed Jun 1, 2022
View reviewed changes
Copy link
Collaborator

@usha1830 usha1830 left a comment

Choose a reason for hiding this comment

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

I have reviewed at high level and given some comments. Please check
Also please attach a P4 program and generated p4info and bfrt json files

control-plane/bfruntime.cpp Outdated
BUG("Invalid oneof case for the match type of table '%1%'", pre.name());
break;
}
addKeyField(keys_json, mf.id(), mf.name(), false /* mandatory */,
Copy link
Collaborator

@usha1830 usha1830 Jun 1, 2022

Choose a reason for hiding this comment

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

indentation

control-plane/p4RuntimeArchStandard.h Outdated
}
};

template<> struct MatchValueLookupTableTraits<Arch::V1MODEL2020> {
Copy link
Collaborator

@usha1830 usha1830 Jun 1, 2022

Choose a reason for hiding this comment

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

This may be just PNA specific. Why do we add it for V1Model

Copy link
Owner Author

@kamleshbhalui kamleshbhalui Jun 2, 2022

Choose a reason for hiding this comment

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

it's easiest to fix compilation rather separating again entire p4info generation mechanism for pna from rest.
anyway it will be compilation error for rest of all arches

Copy link
Collaborator

@usha1830 usha1830 Jun 2, 2022

Choose a reason for hiding this comment

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

ok

control-plane/p4RuntimeArchStandard.h Outdated
MatchValueLookupTable(cstring n, uint32_t kbw, std::vector<param_t> params_list,
uint32_t sz, const IR::IAnnotated* annos = nullptr) :
name(n), key_bitwidth(kbw), params(params_list), size(sz), annotations(annos) {}
static void add_mvlut_param(uint32_t &param_count, std::vector<param_t>* params_list,
Copy link
Collaborator

@usha1830 usha1830 Jun 1, 2022

Choose a reason for hiding this comment

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

I think we should add a new lines between the variable declarations, constructor call and the last static func

@usha1830
Copy link
Collaborator

usha1830 commented Jun 1, 2022

Tests are failing

@kamleshbhalui
Copy link
Owner Author

kamleshbhalui commented Jun 2, 2022

Test will fail here because p4info is coming from another repo called p4runtime

usha1830
usha1830 approved these changes Jun 2, 2022
View reviewed changes
Copy link
Collaborator

@usha1830 usha1830 left a comment

Choose a reason for hiding this comment

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

Looks fine.

psivanup
psivanup approved these changes Jun 2, 2022
View reviewed changes
Copy link
Collaborator

@psivanup psivanup left a comment

Choose a reason for hiding this comment

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

Looks ok to me.
Is it possible to add a testcase to emit/validate p4info, bfrt Json?

@kamleshbhalui
Copy link
Owner Author

kamleshbhalui commented Jun 3, 2022

p4info.txt
bfrt.json.txt

@kamleshbhalui kamleshbhalui force-pushed the ExactMatchValueLookupTable_Extern branch from 71da7a9 to 1d20474 Compare July 20, 2022 08:01
@kamleshbhalui kamleshbhalui changed the title Emit p4info and bfrt for MatchValueLookupTable Emit p4info , bfrt and context for MatchValueLookupTable Jul 21, 2022
@kamleshbhalui kamleshbhalui force-pushed the ExactMatchValueLookupTable_Extern branch 3 times, most recently from 9bb4f12 to acede95 Compare July 27, 2022 07:48
psivanup
psivanup approved these changes Jul 28, 2022
View reviewed changes
Copy link
Collaborator

@psivanup psivanup left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

I'm not sure if open source repo maintainers allow adding MatchValueLookupTable to all archs or only to PNA.

backends/dpdk/dpdkArch.h Outdated
"%1%: expected size and optionally init_val as arguments", d);
}
} else if (externTypeName == "MatchValueLookupTable") {
if (d->arguments->size() != 1 &&
Copy link
Collaborator

@psivanup psivanup Jul 28, 2022

Choose a reason for hiding this comment

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

Hope you check can for boundaries ( > 0 && <= 3) instead of three individual comparisons.

usha1830
usha1830 approved these changes Jul 29, 2022
View reviewed changes
Copy link
Collaborator

@usha1830 usha1830 left a comment

Choose a reason for hiding this comment

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

I have few minor comments. Please check and update

backends/dpdk/dpdkContext.cpp
}
}
}
// for match value lookup tables decls
Copy link
Collaborator

@usha1830 usha1830 Jul 29, 2022

Choose a reason for hiding this comment

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

Can we not combine these two for loops?

backends/dpdk/dpdkContext.h
};

struct LookupMatchAttributes {
std::vector<LookupHwBlocks*> hardware_blocks;
Copy link
Collaborator

@usha1830 usha1830 Jul 29, 2022

Choose a reason for hiding this comment

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

We should change this name to maybe "target_blocks" or "pipeline_blocks". DPDK is a software pipeline, so we should not use hardware. This applies to all the places where "Hardware" is used.

control-plane/p4RuntimeArchStandard.h
(uint32_t)type->width_bits()});
}
}
/// @return the information required to serialize an @instance of register
Copy link
Collaborator

@usha1830 usha1830 Jul 29, 2022

Choose a reason for hiding this comment

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

Comment says : instance of "register", should be MatchValueLookupTable instead?

@kamleshbhalui kamleshbhalui force-pushed the ExactMatchValueLookupTable_Extern branch from 8523529 to 681980d Compare August 25, 2022 07:13
@kamleshbhalui
Emit p4info and bfrt for MatchValueLookupTable
6921cdf 
Co-authored-by: Sivanupandi, Pitchumani <pitchumani.sivanupandi@intel.com>
@kamleshbhalui
Addressed comments
2c2102f
@kamleshbhalui
Emit context json for MatchValueLookupTable externs (#19)
1060a4b 
* Emit context json for MatchValueLookupTable externs
Co-authored-by: Subramanian, Maheswari <maheswari.subramanian@intel.com>
		Saroha, Yogender Singh <yogender.singh.saroha@intel.com>

* Addressed comments

* checks for mirror_profiles

* updated field name

* Remove strict condition for now about structure arg

* use bits not bytes

* Fix some json key names
@kamleshbhalui
Emit extern info for MatchValueLookupTable in context json
45a6971
@kamleshbhalui
Lift condition for checking 3 constructor args
2cee756
@kamleshbhalui
Emit type in bytes and add match value type in snake case
3f7db95
@kamleshbhalui
Fix redudant generation of action param data
b793a66
@kamleshbhalui
Emit key name for MatchValueLookupTable in bfrt and context json
601f2bc
@kamleshbhalui kamleshbhalui force-pushed the ExactMatchValueLookupTable_Extern branch from 681980d to 03c5cbe Compare November 21, 2022 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@usha1830 usha1830 usha1830 approved these changes

@psivanup psivanup psivanup approved these changes

@github-sajan github-sajan Awaiting requested review from github-sajan

@kdholaki kdholaki Awaiting requested review from kdholaki

@maheswari-s maheswari-s Awaiting requested review from maheswari-s

@Sosutha Sosutha Awaiting requested review from Sosutha

@syogender syogender Awaiting requested review from syogender

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kamleshbhalui @usha1830 @psivanup