[PATCH] D52954: Annotate timeline in Instruments with passes and other timed regions.

[Justin Bogner via Phabricator via llvm-commits]

bogner accepted this revision.
bogner added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: jdoerfert.

Is there a more generic term for this than "signpost"? If so, it might be nice to use that to avoid confusion when/if other implementations show up. If not, that's fine.

Overall, looks reasonable to me, other than a few nitpicks.



================
Comment at: cmake/modules/HandleLLVMOptions.cmake:893
+
+option(LLVM_SUPPORT_XCODE_SIGNPOSTS "Enable support for Xcode signposts" OFF)
----------------
Can we detect when this is available rather than make it an option?


================
Comment at: include/llvm/Support/Timer.h:16
 #include "llvm/Support/DataTypes.h"
+#include "llvm/Config/config.h"
 #include <cassert>
----------------
Public headers can't include config.h. We don't install it, so folks using llvm as a library can't see it. That said, do we even need this include?


================
Comment at: lib/Support/Signposts.cpp:84-87
+  Impl = nullptr;
+#if HAVE_ANY_SIGNPOST_IMPL
+  Impl = new SignpostEmitterImpl();
+#endif // if HAVE_ANY_SIGNPOST_IMPL
----------------
Better to do #if/#else here, in case any compilers warn about the redundant assignment (I'm not sure if they do)


================
Comment at: lib/Support/Signposts.cpp:98-99
+#if HAVE_ANY_SIGNPOST_IMPL
+  if (Impl == nullptr)
+    return false;
+  return Impl->isEnabled();
----------------
Can Impl ever be null if HAVE_ANY_SIGNPOST_IMPL? I think we can remove these checks.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52954/new/

https://reviews.llvm.org/D52954