-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Unity][Transform] Handle call_tir_inplace in FuseTIR and FuseOps
#16487
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
call_tir_inplace in FuseTIRcall_tir_inplace in FuseTIR and FuseOps.
Lunderberg
left a comment
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.
Overall, looks good! The main requests for changing are to make it more robust against upstream errors.
| mutator.fused_tir_funcs_.Set(gv, fused_tir); | ||
| const auto& [prim_func, indices] = FusedTIRConstructor::GetFusedTIR(mod, gv); | ||
| mutator.fused_tir_funcs_.Set(gv, prim_func); | ||
| if (!indices.empty()) { |
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.
Nitpick: Using if (indices.size()) instead of if (!indices.empty()) would avoiding double-negatives and make the condition easier to read. (Though, personal preference as if (indices.size()) relies on conversion of non-zero size_t to true.)
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.
Kind of subjective, I think "not empty" is readable (sounds more like how it would be phrased verbally). I'll change it if you prefer it.
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.
Probably more personal preference, so either works.
call_tir_inplace in FuseTIR and FuseOps.call_tir_inplace in FuseTIR and FuseOps
Lunderberg
left a comment
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.
Thank you for the changes, and looks good!
ce9d196 to
111f08e
Compare
This PR handles in-place
PrimFuncs inFuseTIR, namely by propagating the in-place indices. This required a few updates in the two passes but nothing very heavy-duty.