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

put code_gen.h in custom namespace #1104

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

put code_gen.h in custom namespace #1104

wants to merge 3 commits into from

Conversation

calad0i
Copy link
Contributor

@calad0i calad0i commented Oct 29, 2024

Description

Put code_gen.h also in custom namespace, if defined. Tests will be combined into with another PR.

@calad0i calad0i force-pushed the vivado_codegen_namespace branch from d2009dd to 645576e Compare October 29, 2024 23:46
@calad0i calad0i changed the title isolate code_gen in namespace change put code_gen.h in custom namespace Oct 29, 2024
vloncar
vloncar reviewed Oct 30, 2024
View reviewed changes
example-models Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

can you separate this change from the rest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committed by mistake, will revert it

@vloncar
Copy link
Contributor

vloncar commented Oct 30, 2024 •
edited
Loading

ok, sounds reasonable, but may i ask has it caused problems already or you anticipate problems in the future?

@calad0i calad0i force-pushed the vivado_codegen_namespace branch from 193c7f5 to 0d01cc7 Compare October 30, 2024 18:59
@calad0i
Copy link
Contributor Author

calad0i commented Oct 30, 2024

ok, sounds reasonable, but may i ask has it caused problems already or you anticipate problems in the future?

Should be good now. It is not causing problems for me at the moment, but didn't do a thorough test either.
Was wanting to isolate namespaces for jit compiling, but it still doesn't work as files/macro definitions are still conflicting.

@calad0i calad0i added the please test Trigger testing by creating local PR branch label Oct 30, 2024
@vloncar
Copy link
Contributor

vloncar commented Oct 30, 2024

You are JIT-ing the C++/HLS code? 😄

@calad0i
Copy link
Contributor Author

calad0i commented Oct 30, 2024

Yes - it is like 60% time for compiling in some cases, and >200% for some other. Unfortunately, I had to create a separate process for each compilation, which is rather inefficient (need to parsing headers every time, etc).

@calad0i calad0i added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Oct 30, 2024
@calad0i calad0i added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Oct 31, 2024
@jmitrevs jmitrevs added this to the v1.1.0 milestone Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vloncar vloncar vloncar left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@calad0i @vloncar @jmitrevs