Enable pass timings reporting with NewPassManager

[yashssh]

Enable LLVM Pass timings reporting with NewPassManager, to be used by Numba

[gmarkall]

Thanks for the PR! I have left some comments on the diff - mostly I think there's some inconsistencies in the naming of things.

The main thing I'm unclear about is whether it makes sense to disable pass timing when reporting the timings. The old pass manager seems to have report / reset as being distinct from disabling it. What is the reason for the change in API here?

Alternatively, should the set_time_passes() method for the new pass manager also handle both enabling and disabling, and reset_and_report_timings() only reset and report (not disable)?

[gmarkall]

Is there some asymmetry here - do pass instrumentation callbacks need to be unregistered if we've disabled pass timing?

[yashssh]

No. A unique PICallbacks object belongs to PassBuilder object and is used to do stuff like this(hooks to enable/disable pass timings) after the optimisation pipeline has been built.

[gmarkall]

If you start and finish pass timing, and then start pass timing again, do you end up with the callbacks registered twice though?

[yashssh]

TimePassesHandler::registerCallbacks has an early exit which will prevent this.

[gmarkall]

I don't see how that early exist handler helps in this scenario, in which the user at the Python level has written

pb.start_pass_timing()
pb.finish_pass_timing()
pb.start_pass_timing()

At this point pass timing is re-enabled, and it looks like the callbacks will be registered a second time at https://github.com/numba/llvmlite/pull/1163/files#diff-45eef2a2ab57512c9c03ab44fbebc4228722e001d790af044bd2ca511df08414R306

[yashssh]

Ah you are right. Sorry, I read the code wrong.
Right, I tried doing this locally

pb.start_pass_timing()
mpm = pb.getModulePassManager()
mpm.run(module, pb)
report = pb.finish_pass_timing()

pb.start_pass_timing()
mpm.run(module, pb)
report2 = pb.finish_pass_timing()

I get an asserting failure from LLVM:

python: /home/yashwants/work/upstream/numba-llvm16/llvm/lib/IR/PassTimingInfo.cpp:255: void llvm::TimePassesHandler::startPassTimer(StringRef): Assertion `!ActivePassTimer && "should only have one pass timer at a time"' failed.

Which is also consistent with the fact we allow running the optimisation pipeline with the PassManger object only once. Should we try to handle this in Python? Imo we can ignore this.

[gmarkall]

We can't have CPython get killed by a user's Python coding error. We could have a check in start_pass_timing that checks if it's already been called and raises a RuntimeError if it has already been called before, to protect the interpreter.

[yashssh]

I see. I have added a mechanism to prevent enabling time passes multiple times, what do you think?

[yashssh]

The main thing I'm unclear about is whether it makes sense to disable pass timing when reporting the timings. The old pass manager seems to have report / reset as being distinct from disabling it. What is the reason for the change in API here?

Pass timings monitoring was always reset and disabled after the report generation across all the examples I saw. Hence, I though reset_and_report_timings() might should implicitly disable it. But re-reading all the code the APIs do seem confusing because of the duplication and names.

What do you think about

1. Renaming the new APIs to enable_pass_timings and report_reset_and_disable_pass_timings. I don't want to re-use the old set_pass_timings as the reset_and_report_timings API for the NewPassManager "had" to be tied with PassBuilderObject. It might be good idea to mark the old one as deprecated though.
2. Alternatively we can keep the same mechanism as the Legacy PM with a switch API (set_time_passes) and a report API (reset_and_report_timings) both tied to PassBuilder.

[gmarkall]

What do you think about

I'd prefer option 1, but call the functions start_pass_timing and finish_pass_timing, where finish_pass_timing still returns the report. I think it was getting overly complicated putting everything the function does to achieve its goal in its name.

[yashssh]

Thanks for review :) I have updated all the API names as per your suggestions.

[gmarkall]

Thanks for the updates - all updates look good. I have just one more question on the diff: #1163 (comment)

[gmarkall]

I think this is a little clearer (the current message implies there's a way to remove a pass timer, which I think there isn't):

Suggested change

	raise RuntimeError("Pass builder should only have one \
	pass timer at a time")
	raise RuntimeError("Pass timing can only be done once")

[gmarkall]

It's good to check the error is for the expected reason - for example I think

Suggested change

	with self.assertRaises(RuntimeError):
	with self.assertRaisesRegex(RuntimeError, "not enabled"):

should do it.

[gmarkall]

As per suggestion below:

Suggested change

	with self.assertRaises(RuntimeError):
	with self.assertRaisesRegex(RuntimeError, "only be done once"):

[gmarkall]

@yashssh Many thanks for the updates - comments on the diff (should be quick to resolve).

[gmarkall]

I just noticed the new pass manager timing mechanisms don't seem to appear in the documentation - could you add these please? I think adding them to the documentatiuon for the PassBuilder and then linking to that from the "Pass Timings" section will be sufficient.