Skip to content

Only pass buffer location property if in simulation environment #149

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

Merged
merged 2 commits into from
Jul 20, 2022

Conversation

ericxu233
Copy link
Contributor

This addresses #147.
We only pass in the buffer location property if the design is in simulation environment. Also, we have to query the offline mode since that information is not cached anywhere.

@ericxu233 ericxu233 requested review from bsyrowik and pcolberg July 15, 2022 17:20
Copy link
Contributor

@pcolberg pcolberg left a comment

Choose a reason for hiding this comment

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

@ericxu233, you probably want to modify the code in acl_usm.cpp instead, and only pass on the buffer location property when in simulation.

@ericxu233
Copy link
Contributor Author

ericxu233 commented Jul 15, 2022

Yeah, this needs to be changed again.

Copy link
Contributor

@pcolberg pcolberg left a comment

Choose a reason for hiding this comment

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

Thanks @ericxu233, this looks better! I would suggest that you limit the check specifically to the buffer_location property, corresponding to the title of the pull request. You can move the entire initialization of use_offline_only into the if (mem_id) { block on line 152-155, and only append the buffer location property when in simulation. That way the environment variable is only queried if the buffer location property was passed to clHostMemAllocINTEL() etc.

Copy link
Contributor

@pcolberg pcolberg left a comment

Choose a reason for hiding this comment

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

Thanks @ericxu233, this is good to go with the minor modifications detailed below 🙂

@pcolberg
Copy link
Contributor

Perfect 🙂

For the FPGA runtime, we merge pull requests commit by commit, where each commit should be one self-contained change.

Could you squash your commits into a single commit and include the above pull-request description in the commit message?

You can use an interactive rebase to create the single commit:

git rebase -i intel/main
pick f4bc596 Only pass buffer location property if in simulation environemnt
pick 89d114e Modify buffer location property if in simulation environment in acl_usm.cpp
pick b50288c Pass buffer location property only if the environment is simulation
pick c79fde5 Fixed code to pass key-value pair of buffer location property in simulation mode

In the editor, set the subsequent commits to squash or s:

pick f4bc596 Only pass buffer location property if in simulation environemnt
s 89d114e Modify buffer location property if in simulation environment in acl_usm.cpp
s b50288c Pass buffer location property only if the environment is simulation
s c79fde5 Fixed code to pass key-value pair of buffer location property in simulation mode

Save and exit the editor, then polish the commit message in the next editor that opens.

Finally, force-push your branch with the new, single commit:

git push --force-with-lease ericxu233 USM-fix

@pcolberg pcolberg changed the title Only pass buffer location property if in simulation environemnt Only pass buffer location property if in simulation environment Jul 15, 2022
@pcolberg
Copy link
Contributor

pcolberg commented Jul 18, 2022

Thanks @ericxu233 for squashing. I have pushed a cosmetic revision fixing a typo in environment and mentioning your link to the issue. I will merge once integration testing has passed.

Copy link
Contributor

@bsyrowik bsyrowik left a comment

Choose a reason for hiding this comment

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

Looks good, thanks Eric!

A board's MMD may not support passing property lists containing only a
null terminator to aocl_mmd_host_alloc() or aocl_mmd_shared_alloc().

This resolves a regression introducted in intel#141

Signed-off-by: Peter Colberg <[email protected]>
@pcolberg pcolberg force-pushed the USM-fix branch 2 times, most recently from cbc2d03 to 71fe1c4 Compare July 20, 2022 16:38
@pcolberg pcolberg added the bug Something isn't working label Jul 20, 2022
@pcolberg pcolberg added this to the 2022.3 milestone Jul 20, 2022
@pcolberg pcolberg merged commit ba6818b into intel:main Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants