Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fin Perf Eval handles (reboot) #1548
Fin Perf Eval handles (reboot) #1548
Changes from all commits
a30ec12
6a2e3cc
f4d819e
ea8f6cf
7b778a8
a796d83
5f8dee1
b61ef31
f9d855e
1bdd262
935b7cc
f7f0a92
62ca5bd
68a85f5
c5f7d14
ea39f5e
e6ab90b
654bf37
8be64c2
35a79fe
a81e034
d3e2345
cbccc05
7db661d
c8ef874
4586def
df156ae
de09e34
6942f72
3844958
c8a4d82
885cbae
f3aa555
48ca257
2b4f9bc
27fd794
d02f581
b79456e
d4054e0
60e33bb
0790521
917a96d
62893b1
b700918
fa91f56
985e31e
626f182
9ce45e6
c36f85a
588a98e
ca60846
f900d17
c965113
0a5e028
5047b6b
5a24aa2
8101962
aa65bea
a6a8ddd
ff6d73c
614fec1
201f5f1
506b678
c817a3a
1c84bf3
12fe42b
f3e37b3
0af9a40
f4dcb6a
6c759b8
a1b58f9
de6b5e7
c1e7a53
19d1321
0ee2510
d00883a
04ee25b
0354cf4
88bc334
034bbb7
eae32e3
eb9c1c2
85c1469
98cca15
c56cf5e
0a88708
5dc2bc3
52795e0
618d8e2
fc970d5
b9cdafa
5c831b2
9249f38
478d6cb
9caa69b
efa967a
c28e5a9
17f4cef
2fe6369
0e9ca20
3cb8a4f
239b866
587b4cd
e1a2a19
e324cb2
9fb6719
d994195
cbdf4b3
9563a5e
2868d28
80630de
8daecf0
59d1ec0
a5499d9
5fcf352
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The body of this function looks like a modified copy of very similar code from
FindSolutionImpl
(src/include/miopen/find_solution.hpp). Can we generalize this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The objective of this function is to return the parameter string from the performance config. I see the portion that is common here, and most likely it was the example for this addition. They are entirely similar in that they
The main difference being (4) the way to minimize this code would be to create an additional function providing a callback in place of (4).
Disregarding log statements and syntactical spacing there are a total of 4 shared lines. Not sure if it's worth complicating with a callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cderb
Excuse me, but AFAICS this function is not used, so it seems like it does not have an objective expressed in the code ;)
Which is not surprising, because from the architectural point of view, getting tuning config from the database is not a Solver's job.
I think that this method should be removed from AnySolver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we remove this function, I'll need a way to retrieve the serialized performance parameters. Could be the winner from the solver or from a solution. I'm searching for another method, but nothing yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add this function to a PR that will actually use it? This will allow us to expedite the review of this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. If you know SolverId, then it is possible to construct an instance of the Solver and then follow the design pattern of:
https://github.com/ROCmSoftwarePlatform/MIOpen/blob/72d89b40ee97666efaac0a0979a48680bd02f1da/src/include/miopen/find_solution.hpp#L123-L125
I can do this for you in a future PR where you will implement Fin changes. Or you can ask @DrizztDoUrden to assist.
But let's avoid adding unused code in this PR. Is it acceptable for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the key issue is that in order to call serialize on PerformanceConfig, access to the fully substantiated type is required, but occluded behind AnySolver. There needs to be some way of extracting that performance parameter string so that the pdb entry data can be recorded by Fin.
A function producing the performance parameter string would be necessary for the current implementation of performance tuning in Fin to be complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please send me proposed Fin changes (where you call
GetPerfCfgParams
) and I will offer a substitute. This is necessary, becauseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll email the code section to your amd address. Do you still have access?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cderb No, unfortunately. But you can send me mail to DXC (please check your amd inbox).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only initialized but not updated anywhere. Can we make this change when it's really needed (thus following the YAGNI principle)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in fin, at conv_fin.hpp:559
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the problem is that this PR does NOT include changes in conv_fin.hpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be resolved by changes in #1588