Host irs: use Communication directly instead of wrapped inside PostOnStream#2328
Conversation
|
!build --dist |
|
!build |
wujingyue
left a comment
wujingyue
left a comment
There was a problem hiding this comment.
Nice -- so I don't need to make the same change!
| postCommunication(post_ir); | ||
| } else { | ||
| NVF_ERROR(false, "The op cannot be posted on a stream: ", op); | ||
| dispatch(op); |
There was a problem hiding this comment.
This is a change of behavior. Did you intend to do this in another PR?
There was a problem hiding this comment.
I don't see any change in behavior, am I missing smth?
There was a problem hiding this comment.
You are right -- there's no behavior change. There's a subtle difference that the new code won't check the type of the host op in PostOnStream, which I think is fine.
There was a problem hiding this comment.
IIUC the type of the host op is still checked, inside ´OptInDispatch::dispatch'
| postCommunication(post_ir); | ||
| } else { | ||
| NVF_ERROR(false, "The op cannot be posted on a stream: ", op); | ||
| dispatch(op); |
There was a problem hiding this comment.
You are right -- there's no behavior change. There's a subtle difference that the new code won't check the type of the host op in PostOnStream, which I think is fine.
…simplify_comm_remove_post
|
!build |
|
!build |
samnordmann
changed the title
Communicator directly instead of wrapped inside PostOnStreamCommunication directly instead of wrapped inside PostOnStream
2 participants
Bootstrapping for simplifying the design. With this patch, the
Communicationclass is used to post a collective, without the need of wrapping it inside aPostOnStreamIr.There is no functional change, this is only a minor cleanup I would like to upstream before introducing the "collective wait" IR.
To achieve this, we need to add I/O TVs to the
Communication's constructor, and use those I/O directly. This patch aligns with the discussion from #2250 and should lead to further simplification inMultiDeviceExecutor, that I would like to leave to another PR for avoiding conflicts.Note that the
Communicationclass is used in two places: Host IRs andMultiDeviceExecutor. Since the former case should replace the latter eventually, we maintain for now these two usages. InMultiDeviceExecutor, we don't set aCommunication's I/O, that's why those arguments are made optional in the constructor