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.

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

Split psa_switch.h into two files #3144

Merged
merged 4 commits into from
Mar 23, 2022
Merged

Conversation

mihaibudiu
Copy link
Contributor

Signed-off-by: Mihai Budiu mbudiu@vmware.com

This is not really the desired final result, but it is a test that the bmv2-specific parts can be separated from the psa-specific parts of the code. The psa-specific parts should be reused by multiple backends. Probably we should move them to some architecture/ directory.

@osinstom can you see whether this structure removes all bmv2 dependencies from the ebpf backend (except psaProgramStructure.h)?

Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
@mihaibudiu
Copy link
Contributor Author

Today the p4c code structure does not have separate directories for architectures (models?) and targets.
The code specific to an architecture essentially validates that the code is written for the correct architecture and extracts all relevant information for code-generation. Code-generation is target-specific, and should only read the extracted information.

@osinstom
Copy link
Contributor

@mbudiu-vmw thanks for investigating this. I'm getting exactly the same error as on CI: https://github.com/p4lang/p4c/runs/5654254563?check_suite_focus=true.

/home/dev/workspace/p4c-1/backends/bmv2/psa_switch/psaProgramStructure.cpp: In member function ‘virtual bool BMV2::InspectPsaProgram::preorder(const IR::Parameter*)’:
/home/dev/workspace/p4c-1/backends/bmv2/psa_switch/psaProgramStructure.cpp:193:9: error: ‘PsaSwitchExpressionConverter’ has not been declared
  193 |     if (PsaSwitchExpressionConverter::isStandardMetadata(ptName)) {
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

@rst0git
Copy link
Member

rst0git commented Mar 23, 2022

@mbudiu-vmw thanks for investigating this. I'm getting exactly the same error as on CI: https://github.com/p4lang/p4c/runs/5654254563?check_suite_focus=true.

/home/dev/workspace/p4c-1/backends/bmv2/psa_switch/psaProgramStructure.cpp: In member function ‘virtual bool BMV2::InspectPsaProgram::preorder(const IR::Parameter*)’:
/home/dev/workspace/p4c-1/backends/bmv2/psa_switch/psaProgramStructure.cpp:193:9: error: ‘PsaSwitchExpressionConverter’ has not been declared
  193 |     if (PsaSwitchExpressionConverter::isStandardMetadata(ptName)) {
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

isStandardMetadata() is defined in psaSwitch.h.

@rst0git
Copy link
Member

rst0git commented Mar 23, 2022

The psa-specific parts should be reused by multiple backends. Probably we should move them to some architecture/ directory.

Sounds good to me.

@osinstom
Copy link
Contributor

@mbudiu-vmw thanks for investigating this. I'm getting exactly the same error as on CI: https://github.com/p4lang/p4c/runs/5654254563?check_suite_focus=true.

/home/dev/workspace/p4c-1/backends/bmv2/psa_switch/psaProgramStructure.cpp: In member function ‘virtual bool BMV2::InspectPsaProgram::preorder(const IR::Parameter*)’:
/home/dev/workspace/p4c-1/backends/bmv2/psa_switch/psaProgramStructure.cpp:193:9: error: ‘PsaSwitchExpressionConverter’ has not been declared
  193 |     if (PsaSwitchExpressionConverter::isStandardMetadata(ptName)) {
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

isStandardMetadata() is defined in psaSwitch.h.

Right. So, the minimal set of dependencies I've found out to (almost) work is:

  ../bmv2/common/programStructure.cpp
  ../bmv2/psa_switch/psaProgramStructure.cpp
  
  ../bmv2/common/programStructure.h
  ../bmv2/psa_switch/psaSwitch.h
  ../bmv2/psa_switch/psaProgramStructure.h

However, I'm facing the following error:

CMakeFiles/p4c-ebpf.dir/unified_p4c_ebpf_srcs_3.cpp.o:unified_p4c_ebpf_srcs_3.cpp:function EBPF::PSASwitchBackend::convert(IR::ToplevelBlock const*): error: undefined reference to 'VTT for BMV2::ParsePsaArchitecture'
CMakeFiles/p4c-ebpf.dir/unified_p4c_ebpf_srcs_3.cpp.o:unified_p4c_ebpf_srcs_3.cpp:function EBPF::PSASwitchBackend::convert(IR::ToplevelBlock const*): error: undefined reference to 'VTT for BMV2::ParsePsaArchitecture'
CMakeFiles/p4c-ebpf.dir/unified_p4c_ebpf_srcs_3.cpp.o:unified_p4c_ebpf_srcs_3.cpp:function EBPF::PSASwitchBackend::convert(IR::ToplevelBlock const*): error: undefined reference to 'vtable for BMV2::ParsePsaArchitecture'
/usr/bin/ld.gold: the vtable symbol may be undefined because the class is missing its key function

I assume it's because preorder methods for ParsePsaArchitecture are still defined in psaSwitch.cpp: https://github.com/mbudiu-vmw/p4c-clone/blob/split-psa/backends/bmv2/psa_switch/psaSwitch.cpp#L251. If I include psaSwitch.cpp I'm getting an explosion of missing dependencies.

Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
@mihaibudiu
Copy link
Contributor Author

Hopefully my new commit has fixed the missing class problem.

Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
@osinstom
Copy link
Contributor

@mbudiu-vmw I confirm that PSA-eBPF compiles successfully with the set of dependencies that I've pasted above. Feel free to merge this, so that I can rebase and apply these modifications to #3139.

@mihaibudiu
Copy link
Contributor Author

Will do once CI tests are finished

Copy link
Member

@rst0git rst0git left a comment

Choose a reason for hiding this comment

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

LGTM

@mihaibudiu mihaibudiu merged commit c55bdb6 into p4lang:main Mar 23, 2022
@mihaibudiu mihaibudiu deleted the split-psa branch March 23, 2022 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants