-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Unity][Transform] Handle call_tir_inplace
in FuseTIR
and FuseOps
#16487
Conversation
call_tir_inplace
in FuseTIR
call_tir_inplace
in FuseTIR
and FuseOps
.
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
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
PrimFunc
s inFuseTIR
, namely by propagating the in-place indices. This required a few updates in the two passes but nothing very heavy-duty.