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

pre-release fixes #113

Merged
merged 17 commits into from
Aug 25, 2022
Merged

pre-release fixes #113

merged 17 commits into from
Aug 25, 2022

Conversation

franz
Copy link
Collaborator

@franz franz commented Aug 17, 2022 •
edited
Loading

  • adds documentation
  • adds some missing implementations in bitcode library
  • removes doxygen documentation, instead adds a CMake target "gendocs" to generate it

linehill
linehill reviewed Aug 17, 2022
View reviewed changes
LICENSE Outdated Show resolved Hide resolved
linehill
linehill reviewed Aug 17, 2022
View reviewed changes
README.md Show resolved Hide resolved
@franz franz mentioned this pull request Aug 17, 2022
Closed
@pvelesko
Copy link
Collaborator

removes doxygen documentation, instead adds a CMake target "gendocs" to generate it

https://chip-spv.github.io/chip-spv/ depends on having the documents built

@pvelesko
Copy link
Collaborator

Some tests are disabled which now pass

pvelesko
pvelesko reviewed Aug 17, 2022
View reviewed changes
samples/CMakeLists.txt
@@ -92,15 +116,14 @@ set(SAMPLES
11_device
hipStreamSemantics
hipKernelLaunchIsNonBlocking
#hipMultiThreadAddCallback
hipMultiThreadAddCallback
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sample can be removed now

@pvelesko
Copy link
Collaborator

It would be great if you could break this PR into something more manageable. For example, make the license texts as a separate one. Currently, this PR is so big that my browser fails to render. Perhaps just reverting the /docs deletion would suffice.

@franz franz mentioned this pull request Aug 17, 2022
Closed
@Kerilk
Copy link
Contributor

Kerilk commented Aug 17, 2022

Shouldn't GaTech be added to the authors, if code was taken from HIPLZ.

@franz franz force-pushed the various2 branch 2 times, most recently from 87725d9 to b3135cc Compare August 18, 2022 13:46
@franz
Copy link
Collaborator Author

franz commented Aug 18, 2022

@Kerilk added GaTech, @pvelesko split the PR in 2 parts, you can try again

pvelesko
pvelesko requested changes Aug 19, 2022
View reviewed changes
Copy link
Collaborator

@pvelesko pvelesko left a comment

Choose a reason for hiding this comment

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

A couple of things are outdated in regards to disabling tests.
Callbacks are disabled even though they should be fixed now.

A lot of tests that do fail and are known to fail are not disabled.

A few other small things.

samples/hipMultiThreadAddCallback/CMakeLists.txt Outdated Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
samples/cuda_samples/CMakeLists.txt Show resolved Hide resolved
samples/cuda_samples/CMakeLists.txt Show resolved Hide resolved
Features.md Show resolved Hide resolved
Features.md Show resolved Hide resolved
@franz
Fix issue #95: disable failing tests
cbe94ef 
Failing tests are now disabled by default. They can be enabled
at cmake time with cmake option -DENABLE_FAILING_TESTS

Tests that don't compile (currently mainly from samples/cuda-samples/
are also disabled by default. They can be enabled with
-DENABLE_UNCOMPILABLE_TESTS cmake option.
@franz
Add cuda samples that currently don't compile
668df04
@franz
Fix some memleaks in some hip-cuda tests
ccaf550 
don't allocate buffers on each iteration
@franz
Fix issue #87: document unsupported features
080291b
@franz
Add licenses
f97491e
@franz
Add license to some OCML files
5029534
@franz
Add licenses
1c01c6e
@franz
Remove some non-ascii characters from files
ad29cf0
@franz
Add licenses to samples
2853524
@franz
bitcode/OCML: add missing functions for y0/y1/j0/j1 implementations
b8541cc
@franz
Fix issue #110
8ead730
@franz
additional fixes for issue #97
0095a94
@franz
Fix names in LICENSE, add GaTech
93f7038
@franz
Remove src/main.cc, was used only for testing
00c3e62
@pvelesko pvelesko closed this Aug 22, 2022
@pvelesko pvelesko deleted the various2 branch August 22, 2022 05:50
@pvelesko pvelesko restored the various2 branch August 22, 2022 05:51
@pvelesko pvelesko reopened this Aug 22, 2022
@franz franz self-assigned this Aug 22, 2022
@pvelesko
Copy link
Collaborator

@franz you mentioned that you will split this PR to open another one exclusively for labelling tests. Could you please do that or at least share the mechanism by which we will be disabling tests. Working on a PR where I am resolving a lot of failing tests it would be great to have these labels in place

@franz
Copy link
Collaborator Author

franz commented Aug 22, 2022

@pvelesko sure, it's in branch various3 (still WIP). Is there something still needed for this PR ?

@franz
Copy link
Collaborator Author

franz commented Aug 22, 2022

@pvelesko ... commit "CMake check target: add a list of failing HIP tests as regexp" is the one that adds the regexp to disable unit tests.

@pvelesko
Copy link
Collaborator

Is there something still needed for this PR ?

Not from my end. It still shown as being in requested changes state so I assumed you're still working on it. I can go ahead and review again

pvelesko
pvelesko requested changes Aug 23, 2022
View reviewed changes
Features.md Outdated Show resolved Hide resolved
Features.md Show resolved Hide resolved
@franz
Copy link
Collaborator Author

franz commented Aug 23, 2022

I don't know why it's still showing requested changes, i think everything is resolved now.

@pvelesko
Copy link
Collaborator

I don't know why it's still showing requested changes, i think everything is resolved now.

You're supposed to re-request review. Let me run the unit tests and I'll approve

pvelesko
pvelesko approved these changes Aug 23, 2022
View reviewed changes
Copy link
Collaborator

@pvelesko pvelesko left a comment

Choose a reason for hiding this comment

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

hipMultiThreadAddCallback (Not Run) ?
Not sure why the unit tests are conflicting perhaps the base is out of date with main?

Other than that, please review if all the tests pass as expected.

level0_unit_tests_iris.log Outdated
619 - Stress_hipMemcpy_multiDevice-AllAPIs - size_t (Failed)
620 - Stress_hipMemcpy_multiDevice-AllAPIs - long double (Failed)
648 - hipMultiThreadAddCallback (Not Run)
Copy link
Collaborator

Choose a reason for hiding this comment

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

not run? @franz

Copy link
Collaborator Author

@franz franz Aug 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is that the logfile in the repository, or is that from an actual ctest run ? I have checked (by running the tests) and it's enabled. I don't generally update the logfile in the repo as i don't have either of iris / arcticus hardware.

@pvelesko pvelesko merged commit 91e0f2c into main Aug 25, 2022
This was referenced Aug 25, 2022
Closed
Closed
@franz franz deleted the various2 branch March 24, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@linehill linehill linehill left review comments

@pvelesko pvelesko pvelesko approved these changes

Assignees

@franz franz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@franz @pvelesko @Kerilk @linehill