Repository navigation
GPUTracking: replace the merger's goto with helper lambdas - #15945
Conversation
davidrohr
left a comment
davidrohr
left a comment
There was a problem hiding this comment.
Hm, I don't like that we need more instructions, but I do not think that there is a better solution. I do not believe this is performance critical, but I want to check at least that it does not change the performance. An alternative to make the code cleaner could be to use a lambda or a separate function.
|
I will try to run with this and O2/dev today and compare the performance on GPU, but I do not think there will be a difference. If we could have a version with a lambda or a function and return instead of goto, I'd prefer that for the syntax, and I assume also that would not change the performance. |
MSL supports neither goto nor labels. The label sat at the end of the loop body, so each jump was a continue for the outer loop that could not be written as one because it was issued from an inner loop. Two lambdas take over the chain walks that did the jumping: one advances a track along its segment chain and reports whether the target was met on the way, the other reports whether the target sits on either neighbour chain. Each jump then becomes a plain continue at the top level of the loop body. A null target lets the first one also cover the four walks that never tested for one, leaving the file three lines shorter than it is with the goto. Checked against the goto version by transcribing both loop bodies into a harness and comparing the skip decision and the resulting segment and neighbour links of every track over 400000 random link graphs, 71% of which took a skip path and 100000 of which reached the merging stage: no differences.
ktf
changed the title
|
Version with the lambda now pushed in place of the old commit. It's even 3 lines shorter... ;-) |
|
@davidrohr for the record, i think this is the last one from the initial 30+ commits PR. I will now try to enable Metal for real and see how far the reconstruction goes. |
davidrohr
left a comment
davidrohr
left a comment
There was a problem hiding this comment.
I compared performance on RTX 5090 and on MI50, between O2 before the first Vulkan change, current O2/dev, and this PR.
Also checked that this PR does not change tracking results in deterministic mode, and that CPU and GPU give the same results. Looks all good. Merging
2 participants
MSL supports neither goto nor labels. The label sat at the end of the loop body,
so each jump was a continue for the outer loop that could not be written as one
because it was issued from an inner loop.
Two lambdas take over the chain walks that did the jumping: one advances a track
along its segment chain and reports whether the target was met on the way, the
other reports whether the target sits on either neighbour chain. Each jump then
becomes a plain continue at the top level of the loop body. A null target lets
the first one also cover the four walks that never tested for one, leaving the
file three lines shorter than it is with the goto.
Checked against the goto version by transcribing both loop bodies into a harness
and comparing the skip decision and the resulting segment and neighbour links of
every track over 400000 random link graphs, 71% of which took a skip path and
100000 of which reached the merging stage: no differences.