[PATCH] D20735: [Support] Allow nesting paired calls to {start, stop}Timer

Vedant Kumar via llvm-commits llvm-commits at lists.llvm.org
Fri May 27 10:11:36 PDT 2016

vsk added a comment.

@rafael we don't know how hard it would be to change users which expect to be able to nest {start,stop}Timer. In CodeGenAction.cpp alone it seems like there could be many such users.

> The real worry on my side is that I'm not aware of all the possible cases where we rely on this behavior.

Right, that's what I'm concerned about too. I don't see any advantages of keeping the stricter (i.e non-nestable) {start,stop}Timer implementation.

Comment at: include/llvm/Support/Timer.h:122
@@ +121,3 @@
+  TimeRecord getTotalTime() const {
+    assert(RefCount == 0 && "Cannot query active timer");
+    return Time;
davide wrote:
> Any reason why you can't query an active timer?
You'd get back a negative time, since startTimer() does `Time -= CurrentTime`.

Comment at: lib/Support/Timer.cpp:155
@@ -151,3 +154,3 @@
 void Timer::clear() {
-  Running = Triggered = false;
-  Time = StartTime = TimeRecord();
+  RefCount = 0;
+  Triggered = false;
davide wrote:
> hmm, imagine the following sequence:
> startTimer()
> startTimer()
> clear()
> stop()
> stop()
> You hit an assertion, no?
Yes. It seems correct to me that you'd hit an assertion in that case. clear() would reset the total time, so there is nothing to stop.


More information about the llvm-commits mailing list