zackcquic commented on pull request #7952:
URL: https://github.com/apache/tvm/pull/7952#issuecomment-843832260
> I think this is a valuable change and should pave way for more powerful
tools in debugging and profiling TVM compilation. Others have left good
comments, I have a few additional questions:
>
> * what happens when PassContexts are nested? I'm not sure if this is
allowed today but it feels to me like they could compose
> * the `disabled_pass` option feels redundant now for PassContext, perhaps
we should keep it for backwards compatibility but it could be easily replaced
using a `DisableByName` instrument (or something like that).
> * is there a reason why we couldn't overload the superclass (`PassNode`)
`operator()` for doing the before and after instrumentation? the new changes
have some duplicated lines for each type of PassNode.
>
> Related to the last point, we also have a problem with the new design
regarding the previous Pass timing profiling: in particular, as passes can be
nested, **every nested pass invocation must use the `operator()(IRModule,
PassContext)` overload** to ensure the pass is recorded in the timing. I'm
pretty sure this is not the case in a lot of existing passes, so we must fix
this globally for the timing to be precise. But since passes can be called
without a PassContext, it might be possible that there is no PassContext- maybe
there's a global/thread local one? I'm not sure.
>
> Thanks for the work!
Hi @altanh, thanks a lot for reviewing along with these great questions. 👍
**Q. What happens when PassContexts are nested? I'm not sure if this is
allowed today but it feels to me like they could compose.**
A. Yes, PassContext can be nested. Instrument will be active in the same
level (in the view of PassContext).
``` python
def test_print():
x, y, z = [tvm.relay.var(c, shape=(3, 4), dtype="float32") for c in
"xyz"]
e1 = op.add(x, y)
e2 = op.subtract(x, z)
e3 = op.multiply(e1, e1 / e2)
mod = tvm.IRModule.from_expr(e3 + e2)
@tvm.instrument.pass_instrument
class NamePrinter:
def run_before_pass(self, mod, pass_info):
print(pass_info.name)
return True
print_name = NamePrinter()
with tvm.transform.PassContext(instruments=[print_name]):
mod = tvm.relay.transform.AnnotateSpans()(mod)
with tvm.transform.PassContext():
mod = tvm.relay.transform.ToANormalForm()(mod)
mod = tvm.relay.transform.InferType()(mod)
test_print()
# output:
# AnnotateSpans
# InferType # Note: InferType is a nested pass, same pass ctx
with AnnotateSpans. Still instrumented, since finally it goes through
ModulePassNode::operator().
#
# Inner level passes are not printed, since no instruments provided.
```
**Q. The `disabled_pass` option feels redundant now for PassContext, perhaps
we should keep it for backwards compatibility but it could be easily replaced
using a `DisableByName` instrument (or something like that).**
A. Yes, I agree. I left it here. Maybe use another PRs to add/remove these
in the future.
**Q. Is there a reason why we couldn't overload the superclass (`PassNode`)
`operator()` for doing the before and after instrumentation? the new changes
have some duplicated lines for each type of PassNode.**
A. Yes, I aware that.
Basically, this PR is just an enhanced "trace" (```with
PassContext(treace=xxx)```), I just replace how the trace are implemented.
For the reason not to overload superclass, I don't know, but I have a guess,
maybe need someone to confirm:
Currently, only ```FunctionPass, ModulePass, SequentialPass and
PrimFuncPass``` extends ```Pass```.
```FunctionPass``` calls ```InferType``` at the end (see the comments for
detail) while ```SequentialPass``` is just like a wrapper.
In these cases, ```InterType``` and ```SequentialPass``` maybe not want to
be traced/instrumented.
**Q. Since passes can be called without a PassContext, it might be possible
that there is no PassContext- maybe there's a global/thread local one? I'm not
sure.**
A. I thought about that, but I prefer the old "trace" way (No pass context,
no trace/instrument).
If I missed something, please let me know. e.g. There is a way to enable
"trace" globally
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
