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

Runtime option for tuning #3459

Merged
merged 13 commits into from
Nov 5, 2020

Conversation

DavidPoliakoff
Copy link
Contributor

Implements #3454 .

The tuning functionality is highly experimental. Further, people could want to do tuning, but not want Kokkos to tune itself (or user policies). This PR separates the idea of "the Kokkos Tuning infrastructure exists" from "Kokkos tunes itself," the latter of which is a runtime option. I introduced it as a default-off option, though I'm flexible on that

@DavidPoliakoff
Copy link
Contributor Author

Retest this please

core/src/impl/Kokkos_Core.cpp Outdated Show resolved Hide resolved
core/src/Kokkos_Core.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_Core.cpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_Profiling.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_Core.cpp Show resolved Hide resolved
DavidPoliakoff and others added 3 commits October 8, 2020 09:55
Co-authored-by: Damien L-G <dalg24+github@gmail.com>
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

I am pretty much good with it modulo history clean up

Copy link
Member

@crtrott crtrott 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. But can we add a simple test to the initialization tests? I.e. something which sets the InitArgument and then checks that tune_internals has the right value?

@DavidPoliakoff
Copy link
Contributor Author

@crtrott : addressed in 4614a23, but do review the test changes. Our free'ing strategy leaked memory when we introduced arguments which Kokkos shifted (as it does with kokkos-tune-internals), I fixed that

@DavidPoliakoff DavidPoliakoff added this to In progress in Milestone: Release 3.4 via automation Oct 28, 2020
Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

See comment on check logic.

core/src/impl/Kokkos_Core.cpp Outdated Show resolved Hide resolved
@DavidPoliakoff
Copy link
Contributor Author

@dalg24, re history cleanup, comfortable just squash-merging? No other work depends on this

@crtrott crtrott merged commit 847a014 into kokkos:develop Nov 5, 2020
Milestone: Release 3.4 automation moved this from In progress to Done Nov 5, 2020
masterleinad pushed a commit to masterleinad/kokkos that referenced this pull request Nov 9, 2020
* Added "tune_internals" function

* Only tune Kokkos internals if it was requested

* Only declare internal features if internal tuning is enabled

* Fixed logic error in pre_initialize.

++beers_i_owe_dalg24

* Better documentation message in help

* Remove extra space

Co-authored-by: Damien L-G <dalg24+github@gmail.com>

* Update core/src/impl/Kokkos_Core.cpp

Co-authored-by: Damien L-G <dalg24+github@gmail.com>

* Changed tune_kokkos_internals -> tune_internals

* Format

* Added initialization test

* Fixed logic as per @crtrott's comments

Co-authored-by: Damien L-G <dalg24+github@gmail.com>
dalg24 added a commit to dalg24/kokkos that referenced this pull request Jun 3, 2021
Bug was introduced in kokkos#3459
One argument was unitialized causing an heap overflow caught by the
adress sanitizer
dalg24 added a commit to dalg24/kokkos that referenced this pull request Jun 3, 2021
Bug was introduced in kokkos#3459.
One argument was uninitialized which yielded an heap overflow caught by
the adress sanitizer.
masterleinad pushed a commit to masterleinad/kokkos that referenced this pull request Jun 9, 2021
Bug was introduced in kokkos#3459.
One argument was uninitialized which yielded an heap overflow caught by
the adress sanitizer.
masterleinad pushed a commit to masterleinad/kokkos that referenced this pull request Jun 9, 2021
Bug was introduced in kokkos#3459.
One argument was uninitialized which yielded an heap overflow caught by
the adress sanitizer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants