This is an archive of the discontinued LLVM Phabricator instance.

Handle recursion in LLVMIRGeneration Timer
ClosedPublic

Authored by davide on May 27 2016, 1:49 PM.

Details

Summary

See http://reviews.llvm.org/D20735 for more context.
Implementing this clang-side is not as terrible as I originally thought. Maybe needs a test case, but I wasn't able to reduce one easily (yet).

Diff Detail

Repository
rL LLVM

Event Timeline

davide updated this revision to Diff 58833.May 27 2016, 1:49 PM
davide retitled this revision from to Handle recursion in LLVMIRGeneration Timer.
davide updated this object.
davide added reviewers: rafael, vsk.
davide added a subscriber: cfe-commits.
vsk edited edge metadata.May 27 2016, 2:32 PM

Comments on this patch -- The increment and decrement snippets seem like they could be pulled into helper methods. That should make it easier to update all users of LLVMIRGeneration. I wasn't able to come up with a regression test, but I do think this patch needs one.

Overall comments -- My main concern is that this isn't the only affected user of the Timer class. I have a preference for D20735 because it defines the underlying problem away. I'll defer to someone else on this (maybe @rafael?).

vsk added a comment.Jun 8 2016, 10:49 AM

Ping, any updates on this patch?

rafael edited edge metadata.Jun 8 2016, 7:27 PM
rafael added a subscriber: rafael.

Since we found only one user, I think my preference is to handle it there.

Cheers,
Rafael

Richard, can you please take a look at this? The more I look at it the more it seems weird that we can recurse in this case, but I may miss something

This revision was automatically updated to reflect the committed changes.