Merge DeviceStream into CommandEncoder#3264
Conversation
zcbenz
force-pushed
the
encoder-refactor-1
branch
from
b4c6677 to
3d01c86
Compare
angeloskath
left a comment
angeloskath
left a comment
There was a problem hiding this comment.
Sorry for taking long to review this.
It looks really good. I think the merging is helpful even to clean things up a bit. I have one comment that is more conceptual than practical and I apologize in advance if it is too pedantic. Feel free to ignore it.
The CommandEncoder used to represent the command encoder. A transient object that encodes commands to the GPU and then goes away. The stream was the persistent object. As a result there was a natural place to put the outputs of previous command encoders while keeping the within command encoder inputs outputs in the CommandEncoder object. It is also the reason you felt the need to change the map output to fence to prev_fences_ from outputs.
Anyway, I don't have a good solution tbh. Perhaps adding a comment on line 111 that explains that this block of members is to synchronize the kernels within a single MTL::ComputeCommandEncoder and maybe rename prev_fences_ to prev_ce_outputs_ ?
zcbenz
force-pushed
the
encoder-refactor-1
branch
from
3d01c86 to
0d67d84
Compare
2 participants
Refs #3078, #3216.
Merge the members of
DeviceStreamintoCommandEncoder, and move implementations of encoding methods fromDevicetoCommandEncoder. With this change we would only need to makeget_command_encoderthread safe to be able to run multi-streams in multi-threads.