-
Notifications
You must be signed in to change notification settings - Fork 68
[HostIR] Print index definitions in HostIrContainer::print
#5327
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
Conversation
|
Review updated until commit 8f4be43 Description
Changes walkthrough 📝
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
!test |
Can you remind me why they aren't part of topLevelExprs? Analogously, imagine you write a C++ loop
|
I am not sure to understand your comment correctly. Index computations are not part of topLevelExprs just because they don't need to. When we call The example you wrote looks good to me, but I am not sure to understand what you suggest by it. The example I provided in the PR description explains well the use case for the present patch. |
That's fair enough and LGTM. I recall that for MultiDeviceExecutor we also have to find and When Hanlin worked on host IR JIT, we realized that finding what indices to invalidate at "run" time creates problems for host latency. So, for the FusionExecutorCache integration, I let host IR lowering find these index calculations at "compile" time and put them in the scope of the for loop. This is done on a separate code path so doesn't affect MultiDeviceExecutor. I didn't get a chance to check with you -- hence my question earlier. |
Ok, now I understand. I like the idea of what was done for host IR JIT. Do you have a pointer to the PR? It would make sense to do the same in MultiDeviceExecutor. |
|
!test |
Improve printing of HostIrContainer by printing the index computations which are not explicitly part of the
topLevelExprs.Example from #5259