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

Set intersection #113

Merged
merged 4 commits into from
May 17, 2014
Merged

Set intersection #113

merged 4 commits into from
May 17, 2014

Conversation

roshanr95
Copy link
Contributor

Added new kernels, algorithm and test for set_intersection

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) when pulling 663579c on roshanr95:set_intersection into 2fffd98 on kylelutz:develop.


for (unsigned int i = 0; i < tile_a.size(); ++i)
{
std::cout<<tile_a[i]<<" "<<tile_b[i]<<std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

debugging code? should be removed.

@kylelutz
Copy link
Collaborator

Looks good! The tests pass on my machine. Left a few inline comments. After those are addressed it should be good to merge.

@roshanr95
Copy link
Contributor Author

Oh sorry for the kind of half-baked version, forgot to mention that I actually wanted you to test it on your machine since I had issues with it passing on my machine and failing on travis. Anyway good to hear that it is working on your setup as well. Will make a final version fit for merging soon...

Added kernel to tile sets based on a balanced path
Added kernel to compact the results of set kernels into actual sets
@roshanr95
Copy link
Contributor Author

Done :)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c522d9a on roshanr95:set_intersection into 2fffd98 on kylelutz:develop.


exclusive_scan(counts.begin(), counts.end(), counts.begin(), queue);

queue.finish();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this shouldn't be needed (calling clFinish() can be expensive)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is needed to determine the number of elements in the intersection. Without waiting for the queue to finish, how would I be able to return an iterator pointing to end of intersection?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ahh, i see. so instead of calling finish() here, down below when you get counts.back(), I would change it to use the internal detail::read_single_value() function which takes a queue argument and enqueues the operation. so something like: return result + detail::read_single_value(counts.get_buffer(), counts.size() - 1, queue);. This will still block at the end, but will remove the need for two separate host-device synchronization points (which happens now when you explicitly call finish()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Neat!

@kylelutz
Copy link
Collaborator

Cool! Left a few more comments. I'm away right now but I should be able to test this on my AMD machine early next week.

Change tile_size from string to int, use read_single_value instead of
finish
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) when pulling 1bb652c on roshanr95:set_intersection into 2fffd98 on kylelutz:develop.

@roshanr95
Copy link
Contributor Author

Done.. :)

kylelutz added a commit that referenced this pull request May 17, 2014
@kylelutz kylelutz merged commit e44f79c into boostorg:develop May 17, 2014
@kylelutz
Copy link
Collaborator

Awesome! Merged!

@roshanr95 roshanr95 deleted the set_intersection branch May 18, 2014 16:01
@kylelutz kylelutz mentioned this pull request May 18, 2014
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