[PATCH] D119825: [clang][lex] Introduce `SearchDirIndex` to usage tracking code

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added a comment.

Definitely a lot nicer 




Comment at: clang/include/clang/Lex/HeaderSearch.h:185
+  /// Get search directory stored at the index.
+  DirectoryLookup (HeaderSearch ) const;
+};

I would have expected these to be methods on `HeaderSearch`. Is there a 
particular reason why these are methods on `SearchDirIdx`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119825

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119722: [clang][lex] Use `SearchDirIterator` types in for loops

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen accepted this revision.
ahoppen added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/Lex/HeaderSearch.cpp:1450
 Optional HeaderSearch::searchDirIdx(const DirectoryLookup ) const 
{
-  for (unsigned I = 0; I < SearchDirs.size(); ++I)
-if ([I] == )
-  return I;
-  return None;
+  return  - &*SearchDirs.begin();
 }

jansvoboda11 wrote:
> ahoppen wrote:
> > Could we change this function to return an `unsigned` instead of 
> > `Optional` now?
> > 
> > Also, is ` - &*SearchDirs.begin()` safe and doesn’t trigger the issues 
> > we saw previously if start and end are allocated in different memory 
> > regions, because I don’t know.
> Yes, updating the return type makes sense now, thanks!
> 
> Yes, this should be safe as of this patch, since we're still storing 
> `DirectoryLookup` objects in `std::vector`. I'll be updating 
> this code when we change the storage (separate `std::vector` 
> for quoted, angled and system search paths).
OK, great. Makes sense. :thumbsup:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119708: [clang][lex] Remove `PPCallbacks::FileNotFound()`

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen accepted this revision.
ahoppen added a comment.
This revision is now accepted and ready to land.

Assuming that this is indeed never used, removing dead code always sounds good 
to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119708

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119722: [clang][lex] Use `SearchDirIterator` types in for loops

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added a comment.

Nice. I think this reads a lot better than the index-based implementation.




Comment at: clang/lib/Lex/HeaderSearch.cpp:1450
 Optional HeaderSearch::searchDirIdx(const DirectoryLookup ) const 
{
-  for (unsigned I = 0; I < SearchDirs.size(); ++I)
-if ([I] == )
-  return I;
-  return None;
+  return  - &*SearchDirs.begin();
 }

Could we change this function to return an `unsigned` instead of 
`Optional` now?

Also, is ` - &*SearchDirs.begin()` safe and doesn’t trigger the issues we 
saw previously if start and end are allocated in different memory regions, 
because I don’t know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119722

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119721: [clang][lex] Use `ConstSearchDirIterator` in lookup cache

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added inline comments.



Comment at: clang/lib/Lex/HeaderSearch.cpp:710
+  CacheLookup.HitIt = HitIt;
+  noteLookupUsage(&*HitIt - &*search_dir_begin(), Loc);
 }

I haven’t looked into this in total details but `&*` looks a little awkward to 
me. Dereference a pointer/iteration and then get its pointer value back? 

Wouldn’t this hit the same issue that we saw before if `serach_dir_begin` is 
allocated in a different bump allocator begin than `HitIt`?

If possible, would `std::distance` communicate the intent more clearly?



Comment at: clang/lib/Lex/HeaderSearch.cpp:982
 
+  ConstSearchDirIterator NextIt = [](auto ItCopy) { return ++ItCopy; }(It);
+

What’s the reason that this can’t be? The current lambda-based implementation 
looks a little over-complicated to me. But maybe I’m missing something.
```
ConstSearchDirIterator NextIt = ItCopy;
++NextIt;
```

or even something equivalent to 
```
ConstSearchDirIterator NextIt = std::next(ItCopy);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119721

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119716: [clang][lex] NFC: De-duplicate some #include_next logic

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added inline comments.



Comment at: clang/include/clang/Lex/Preprocessor.h:2207
+   const FileEntry *) const;
+
   /// Install the standard preprocessor pragmas:

I there a reason why this uses an out parameter instead of a returning a 
`std::pair` or something similar? If yes, I think it would be good to document 
which parameters are out parameters. `IncludeNextTok` looks suspiciously like 
an out-parameter as well but is not AFAICT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119716

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117312: [clang][lex] NFC: Simplify calls to `LookupFile`

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen accepted this revision.
ahoppen added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117312

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D117309: [clang] NFC: Remove unused `DirectoryLookup`

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen accepted this revision.
ahoppen added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117309

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116750: [clang][lex] Keep references to `DirectoryLookup` objects up-to-date

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added inline comments.



Comment at: clang/include/clang/Lex/HeaderSearch.h:179
   /// directory is suppressed.
-  std::vector SearchDirs;
-  /// Whether the DirectoryLookup at the corresponding index in SearchDirs has
-  /// been successfully used to lookup a file.
-  std::vector SearchDirsUsage;
+  std::vector SearchDirs;
+  /// Set of SearchDirs that have been successfully used to lookup a file.

jansvoboda11 wrote:
> ahoppen wrote:
> > I haven’t tried it but is there a particular reason why this can’t be a 
> > `const DirectoryLookup *`?
> While iterating over `SearchDirs`, the elements can be passed to 
> `HeaderSearch::loadSubdirectoryModuleMaps` that mutates them.
OK. Makes sense. Thanks.



Comment at: clang/lib/Lex/HeaderSearch.cpp:323
+
+if (SearchDirs[Idx]->isFramework()) {
   // Search for or infer a module map for a framework. Here we use

jansvoboda11 wrote:
> ahoppen wrote:
> > Nitpick: `SearchDirs[Idx]` can be simplified to `SearchDir->isFramework()`. 
> > Similarly below.
> `SearchDir` will be removed in the following patch: D113676.
Ah, OK. In that case it makes sense to keep using `SearchDirs[Idx]`.



Comment at: clang/unittests/Lex/HeaderSearchTest.cpp:276
+std::vector ExpectedSearchDirUsageAfterM2{false, true, false};
+EXPECT_EQ(Search.getSearchDirUsage(), ExpectedSearchDirUsageAfterM2);
+std::vector ExpectedUserEntryUsageAfterM2{false, true, false};

jansvoboda11 wrote:
> ahoppen wrote:
> > Wouldn’t it be cleaner to just check that `UsedSearchDirs` only contains a 
> > single element and that it’s name is `/M2`? In that case we could also 
> > remove `getSearchDirUsage`.
> Maybe I'm misunderstanding you, but I don't think so. We'd still need 
> accessor for `HeaderSearch::UsedSearchDirs` and we don't have the expected 
> `DirectoryLookup *` lying around, making matching more cumbersome:
> 
> ```
> const llvm::DenseSet  =
> Search.getUsedSearchDirs();
> EXPECT_EQ(UsedSearchDirs.size(), 2);
> EXPECT_EQ(1, llvm::count_if(UsedSearchDirs, [](const auto *SearchDir) {
> return SearchDir->getName() == "/M1";
>   }));
> EXPECT_EQ(1, llvm::count_if(UsedSearchDirs, [](const auto *SearchDir) {
> return SearchDir->getName() == "/M2";
>   }));
> ```
> 
> or
> 
> ```
> llvm::DenseSet UsedSearchDirsStr;
> for (const auto *SearchDir : Search.getUsedSearchDirs())
>   UsedSearchDirsStr.insert(SearchDir->getName());
> EXPECT_EQ(UsedSearchDirsStr, (llvm::DenseSet{"/M1", "/M2"}));
> ```
> 
> I think having bit-vector, whose indices correspond to the directory names 
> (`"/M{i}"`), and using `operator==` for matching is simpler.
> 
> Let me know if you had something else in mind.
I just don’t like the bit-vectors and basically thought of the second option 
you were suggesting, but maybe that’s really just personal taste. If you’d like 
to keep the bit-vector, could you change the comment of `getSearchDirUsage` to 
something like 
```
/// Return a vector of length \c SearchDirs.size() that indicates for each 
search directory whether it was used.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116750: [clang][lex] Keep references to `DirectoryLookup` objects up-to-date

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added a comment.

I like this a lot better. Some comments inline.




Comment at: clang/include/clang/Lex/HeaderSearch.h:179
   /// directory is suppressed.
-  std::vector SearchDirs;
-  /// Whether the DirectoryLookup at the corresponding index in SearchDirs has
-  /// been successfully used to lookup a file.
-  std::vector SearchDirsUsage;
+  std::vector SearchDirs;
+  /// Set of SearchDirs that have been successfully used to lookup a file.

I haven’t tried it but is there a particular reason why this can’t be a `const 
DirectoryLookup *`?



Comment at: clang/lib/Lex/HeaderSearch.cpp:323
+
+if (SearchDirs[Idx]->isFramework()) {
   // Search for or infer a module map for a framework. Here we use

Nitpick: `SearchDirs[Idx]` can be simplified to `SearchDir->isFramework()`. 
Similarly below.



Comment at: clang/unittests/Lex/HeaderSearchTest.cpp:276
+std::vector ExpectedSearchDirUsageAfterM2{false, true, false};
+EXPECT_EQ(Search.getSearchDirUsage(), ExpectedSearchDirUsageAfterM2);
+std::vector ExpectedUserEntryUsageAfterM2{false, true, false};

Wouldn’t it be cleaner to just check that `UsedSearchDirs` only contains a 
single element and that it’s name is `/M2`? In that case we could also remove 
`getSearchDirUsage`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116751: [clang][lex] NFC: Extract module creation into function

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen accepted this revision.
ahoppen added a comment.
This revision is now accepted and ready to land.

Ah, in that case it makes sense as-is. I just assumed they lived in the same 
library.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116751

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116750: [clang][lex] Keep search directory indices up-to-date

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added a comment.

I would prefer to use `std::shared_ptr` instead of `unsigned`. 
IIUC we already basically have a level of indirection because if we want to get 
the `DirectoryLookup`, we need to find it by its index in `SearchDirs`. And 
updating the indices just seems like a hack to me that can be solved more 
cleanly.

Wouldn’t we also need to update the indices in `SearchDirsUsage` and 
`ModuleToSearchDirIdx`? Or replace those by `std::shared_ptr` 
as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116751: [clang][lex] NFC: Extract module creation into function

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added a comment.

I just want to make sure that we don’t accidentally introduce a new 
instantiation of a `Module` that doesn’t go through `makeModule` in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116751

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116750: [clang][lex] Keep search directory indices up-to-date

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added a comment.

Adjusting the indices seem pretty fragile to me. Any reason why you wanted to 
stick with indices as keys instead of switching to something else like I 
suggested here ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116751: [clang][lex] NFC: Extract module creation into function

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added a comment.

I suppose the idea is that all `Module` creations should go through 
`makeModule`, right? In that case I think we should either

- make the `Module` constructor private and `ModuleMap` a friend of `Module`
- or at least add a doc comment to the `Module` constructor that says `Module`s 
should only be created using `ModuleMap::makeModule`.

Or are there other places that also create `Module`s but are not supposed to go 
through `ModuleMap::makeModule`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116751

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added inline comments.



Comment at: clang/lib/Lex/HeaderSearch.cpp:95
+  if (HS.CurrentSearchPathIdx != ~0U)
+HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx;
+}

These indices will be out of date if the search paths are changed via 
`HeaderSearch::SetSearchPaths` or `HeaderSearch::AddSearchPath` after the first 
module map has been loaded. One could probably adjust the indices in 
`AddSearchPath` but maybe it’s easier to not use the index into `SearchDirs` as 
the key. An alternative suggestion would be to use 
`std::shared_ptr` instead, changing some of the data 
structures as follows:
```
- std::vector SearchDirs
+ std::vector> SearchDirs

- std::vector SearchDirsUsage;
+ std::map, bool> SearchDirsUsage; 

- llvm::DenseMap ModuleToSearchDirIdx;
+ llvm::DenseMap> 
ModuleToSearchDirIdx;

- llvm::DenseMap SearchDirToHSEntry;
+ llvm::DenseMap, unsigned> 
SearchDirToHSEntry; 
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added inline comments.



Comment at: clang/lib/Lex/HeaderSearch.cpp:94
+  // Module map parsing initiated by header search.
+  if (HS.CurrentSearchPathIdx != ~0U)
+HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx;

jansvoboda11 wrote:
> ahoppen wrote:
> > When would the `moduleMapModuleCreated` be called while 
> > `CurrentSearchPathIdx == ~0U`? Could this be an `assert` instead?
> This happens whenever any of the `ModuleMap` member functions that create new 
> `Module` instances are called outside of `HeaderSearch`.
> 
> The `MMCallback` callback is basically "global" (present for the whole 
> lifetime of `ModuleMap`), so that we don't have to repeatedly 
> register/deregister it in `HeaderSearch::lookupModule`.
Is there any reasonable case where module maps would be created without 
`HeaderSearch` triggering the creation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added a comment.

LGTM overall. I’ve got a few questions/remarks inline.




Comment at: clang/lib/Lex/HeaderSearch.cpp:94
+  // Module map parsing initiated by header search.
+  if (HS.CurrentSearchPathIdx != ~0U)
+HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx;

When would the `moduleMapModuleCreated` be called while `CurrentSearchPathIdx 
== ~0U`? Could this be an `assert` instead?



Comment at: clang/lib/Lex/HeaderSearch.cpp:264
+  if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps) {
+noteModuleLookupUsage(Module, ImportLoc);
 return Module;

Just a thought: Could we move `noteModuleLookupUsage` into `findModule`? IIUC 
that would guarantee that we’re catching all cases and we wouldn’t need to call 
`noteModuleLookupUsage ` from both overloads of `lookupModule`.



Comment at: clang/lib/Lex/ModuleMap.cpp:851
   new Module("", Loc, Parent, /*IsFramework*/ false,
  /*IsExplicit*/ true, NumCreatedModules++);
   Result->Kind = Module::PrivateModuleFragment;

Maybe a stupid question but why don’t we need to call the callback e.g. here 
(or on the other `new Module`) calls in this file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102001: [Index] Ignore nullptr decls for indexing

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen created this revision.
Herald added a subscriber: arphaman.
ahoppen requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We can end up with a call to `indexTopLevelDecl(D)` with `D == nullptr` in 
non-assert builds e.g. when indexing a module in `indexModule` and

- `ASTReader::GetDecl` returns `nullptr` if `Index >= DeclsLoaded.size()`, thus 
returning `nullptr`

> `ModuleDeclIterator::operator*` returns `nullptr`
===

> we call `IndexCtx.indexTopLevelDecl` with `nullptr`
=

Be resilient and just ignore the `nullptr` decls during indexing.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102001

Files:
  clang/lib/Index/IndexDecl.cpp


Index: clang/lib/Index/IndexDecl.cpp
===
--- clang/lib/Index/IndexDecl.cpp
+++ clang/lib/Index/IndexDecl.cpp
@@ -759,7 +759,7 @@
 }
 
 bool IndexingContext::indexTopLevelDecl(const Decl *D) {
-  if (D->getLocation().isInvalid())
+  if (!D || D->getLocation().isInvalid())
 return true;
 
   if (isa(D))


Index: clang/lib/Index/IndexDecl.cpp
===
--- clang/lib/Index/IndexDecl.cpp
+++ clang/lib/Index/IndexDecl.cpp
@@ -759,7 +759,7 @@
 }
 
 bool IndexingContext::indexTopLevelDecl(const Decl *D) {
-  if (D->getLocation().isInvalid())
+  if (!D || D->getLocation().isInvalid())
 return true;
 
   if (isa(D))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96049: [Timer] On macOS count number of executed instructions

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added inline comments.



Comment at: llvm/CMakeLists.txt:656
+  list(APPEND CMAKE_REQUIRED_LIBRARIES proc)
+endif()
+

thakis wrote:
> also, any reason to not do this in `llvm/cmake/config-ix.cmake` where all the 
> other checks like this are?
No, I just didn't know about `llvm/cmake/config-ix.cmake`. I’ll move it there. 
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96049

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96049: [Timer] On macOS count number of executed instructions

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen updated this revision to Diff 321467.
ahoppen added a comment.

Fixed an issue caused by me using an old revions as the commit's base.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96049

Files:
  
clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-store-check-profile-one-tu.cpp
  llvm/CMakeLists.txt
  llvm/include/llvm/Config/config.h.cmake
  llvm/include/llvm/Support/Timer.h
  llvm/lib/Support/Timer.cpp

Index: llvm/lib/Support/Timer.cpp
===
--- llvm/lib/Support/Timer.cpp
+++ llvm/lib/Support/Timer.cpp
@@ -13,6 +13,7 @@
 #include "llvm/Support/Timer.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Config/config.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
@@ -24,6 +25,14 @@
 #include "llvm/Support/raw_ostream.h"
 #include 
 
+#if HAVE_UNISTD_H
+#include 
+#endif
+
+#ifdef HAVE_PROC_PID_RUSAGE
+#include 
+#endif
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -120,6 +129,17 @@
   return sys::Process::GetMallocUsage();
 }
 
+static uint64_t getCurInstructionsExecuted() {
+#if defined(HAVE_UNISTD_H) && defined(HAVE_PROC_PID_RUSAGE) && \
+defined(RUSAGE_INFO_V4)
+  struct rusage_info_v4 ru;
+  if (proc_pid_rusage(getpid(), RUSAGE_INFO_V4, (rusage_info_t *)) == 0) {
+return ru.ri_instructions;
+  }
+#endif
+  return 0;
+}
+
 TimeRecord TimeRecord::getCurrentTime(bool Start) {
   using Seconds = std::chrono::duration>;
   TimeRecord Result;
@@ -128,9 +148,11 @@
 
   if (Start) {
 Result.MemUsed = getMemUsage();
+Result.InstructionsExecuted = getCurInstructionsExecuted();
 sys::Process::GetTimeUsage(now, user, sys);
   } else {
 sys::Process::GetTimeUsage(now, user, sys);
+Result.InstructionsExecuted = getCurInstructionsExecuted();
 Result.MemUsed = getMemUsage();
   }
 
@@ -180,6 +202,8 @@
 
   if (Total.getMemUsed())
 OS << format("%9" PRId64 "  ", (int64_t)getMemUsed());
+  if (Total.getInstructionsExecuted())
+OS << format("%9" PRId64 "  ", (int64_t)getInstructionsExecuted());
 }
 
 
@@ -339,6 +363,8 @@
   OS << "   ---Wall Time---";
   if (Total.getMemUsed())
 OS << "  ---Mem---";
+  if (Total.getInstructionsExecuted())
+OS << "  ---Instr---";
   OS << "  --- Name ---\n";
 
   // Loop through all of the timing data, printing it out.
@@ -433,6 +459,10 @@
   OS << delim;
   printJSONValue(OS, R, ".mem", T.getMemUsed());
 }
+if (T.getInstructionsExecuted()) {
+  OS << delim;
+  printJSONValue(OS, R, ".instr", T.getInstructionsExecuted());
+}
   }
   TimersToPrint.clear();
   return delim;
Index: llvm/include/llvm/Support/Timer.h
===
--- llvm/include/llvm/Support/Timer.h
+++ llvm/include/llvm/Support/Timer.h
@@ -24,12 +24,15 @@
 class raw_ostream;
 
 class TimeRecord {
-  double WallTime;   ///< Wall clock time elapsed in seconds.
-  double UserTime;   ///< User time elapsed.
-  double SystemTime; ///< System time elapsed.
-  ssize_t MemUsed;   ///< Memory allocated (in bytes).
+  double WallTime;   ///< Wall clock time elapsed in seconds.
+  double UserTime;   ///< User time elapsed.
+  double SystemTime; ///< System time elapsed.
+  ssize_t MemUsed;   ///< Memory allocated (in bytes).
+  uint64_t InstructionsExecuted; ///< Number of instructions executed
 public:
-  TimeRecord() : WallTime(0), UserTime(0), SystemTime(0), MemUsed(0) {}
+  TimeRecord()
+  : WallTime(0), UserTime(0), SystemTime(0), MemUsed(0),
+InstructionsExecuted(0) {}
 
   /// Get the current time and memory usage.  If Start is true we get the memory
   /// usage before the time, otherwise we get time before memory usage.  This
@@ -42,6 +45,7 @@
   double getSystemTime() const { return SystemTime; }
   double getWallTime() const { return WallTime; }
   ssize_t getMemUsed() const { return MemUsed; }
+  uint64_t getInstructionsExecuted() const { return InstructionsExecuted; }
 
   bool operator<(const TimeRecord ) const {
 // Sort by Wall Time elapsed, as it is the only thing really accurate
@@ -49,16 +53,18 @@
   }
 
   void operator+=(const TimeRecord ) {
-WallTime   += RHS.WallTime;
-UserTime   += RHS.UserTime;
+WallTime += RHS.WallTime;
+UserTime += RHS.UserTime;
 SystemTime += RHS.SystemTime;
-MemUsed+= RHS.MemUsed;
+MemUsed += RHS.MemUsed;
+InstructionsExecuted += RHS.InstructionsExecuted;
   }
   void operator-=(const TimeRecord ) {
-WallTime   -= RHS.WallTime;
-UserTime   -= RHS.UserTime;
+WallTime -= RHS.WallTime;
+UserTime -= RHS.UserTime;
 SystemTime -= RHS.SystemTime;
-MemUsed-= RHS.MemUsed;
+MemUsed -= 

[PATCH] D96049: [Timer] On macOS count number of executed instructions

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen created this revision.
Herald added subscribers: dexonsmith, hiraditya, mgorny.
ahoppen requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

In addition to wall time etc. this should allow us to get less noisy
values for time measurements.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96049

Files:
  
clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-store-check-profile-one-tu.cpp
  llvm/CMakeLists.txt
  llvm/include/llvm/Config/config.h.cmake
  llvm/include/llvm/Support/Timer.h
  llvm/lib/Support/Timer.cpp

Index: llvm/lib/Support/Timer.cpp
===
--- llvm/lib/Support/Timer.cpp
+++ llvm/lib/Support/Timer.cpp
@@ -13,6 +13,7 @@
 #include "llvm/Support/Timer.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Config/config.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
@@ -24,6 +25,14 @@
 #include "llvm/Support/raw_ostream.h"
 #include 
 
+#if HAVE_UNISTD_H
+#include 
+#endif
+
+#ifdef HAVE_PROC_PID_RUSAGE
+#include 
+#endif
+
 using namespace llvm;
 
 // This ugly hack is brought to you courtesy of constructor/destructor ordering
@@ -120,6 +129,17 @@
   return sys::Process::GetMallocUsage();
 }
 
+static uint64_t getCurInstructionsExecuted() {
+#if defined(HAVE_UNISTD_H) && defined(HAVE_PROC_PID_RUSAGE) && \
+defined(RUSAGE_INFO_V4)
+  struct rusage_info_v4 ru;
+  if (proc_pid_rusage(getpid(), RUSAGE_INFO_V4, (rusage_info_t *)) == 0) {
+return ru.ri_instructions;
+  }
+#endif
+  return 0;
+}
+
 TimeRecord TimeRecord::getCurrentTime(bool Start) {
   using Seconds = std::chrono::duration>;
   TimeRecord Result;
@@ -128,9 +148,11 @@
 
   if (Start) {
 Result.MemUsed = getMemUsage();
+Result.InstructionsExecuted = getCurInstructionsExecuted();
 sys::Process::GetTimeUsage(now, user, sys);
   } else {
 sys::Process::GetTimeUsage(now, user, sys);
+Result.InstructionsExecuted = getCurInstructionsExecuted();
 Result.MemUsed = getMemUsage();
   }
 
@@ -180,6 +202,8 @@
 
   if (Total.getMemUsed())
 OS << format("%9" PRId64 "  ", (int64_t)getMemUsed());
+  if (Total.getInstructionsExecuted())
+OS << format("%9" PRId64 "  ", (int64_t)getInstructionsExecuted());
 }
 
 
@@ -339,6 +363,8 @@
   OS << "   ---Wall Time---";
   if (Total.getMemUsed())
 OS << "  ---Mem---";
+  if (Total.getInstructionsExecuted())
+OS << "  ---Instr---";
   OS << "  --- Name ---\n";
 
   // Loop through all of the timing data, printing it out.
@@ -433,6 +459,10 @@
   OS << delim;
   printJSONValue(OS, R, ".mem", T.getMemUsed());
 }
+if (T.getInstructionsExecuted()) {
+  OS << delim;
+  printJSONValue(OS, R, ".instr", T.getInstructionsExecuted());
+}
   }
   TimersToPrint.clear();
   return delim;
Index: llvm/include/llvm/Support/Timer.h
===
--- llvm/include/llvm/Support/Timer.h
+++ llvm/include/llvm/Support/Timer.h
@@ -24,12 +24,15 @@
 class raw_ostream;
 
 class TimeRecord {
-  double WallTime;   ///< Wall clock time elapsed in seconds.
-  double UserTime;   ///< User time elapsed.
-  double SystemTime; ///< System time elapsed.
-  ssize_t MemUsed;   ///< Memory allocated (in bytes).
+  double WallTime;   ///< Wall clock time elapsed in seconds.
+  double UserTime;   ///< User time elapsed.
+  double SystemTime; ///< System time elapsed.
+  ssize_t MemUsed;   ///< Memory allocated (in bytes).
+  uint64_t InstructionsExecuted; ///< Number of instructions executed
 public:
-  TimeRecord() : WallTime(0), UserTime(0), SystemTime(0), MemUsed(0) {}
+  TimeRecord()
+  : WallTime(0), UserTime(0), SystemTime(0), MemUsed(0),
+InstructionsExecuted(0) {}
 
   /// Get the current time and memory usage.  If Start is true we get the memory
   /// usage before the time, otherwise we get time before memory usage.  This
@@ -42,6 +45,7 @@
   double getSystemTime() const { return SystemTime; }
   double getWallTime() const { return WallTime; }
   ssize_t getMemUsed() const { return MemUsed; }
+  uint64_t getInstructionsExecuted() const { return InstructionsExecuted; }
 
   bool operator<(const TimeRecord ) const {
 // Sort by Wall Time elapsed, as it is the only thing really accurate
@@ -49,16 +53,18 @@
   }
 
   void operator+=(const TimeRecord ) {
-WallTime   += RHS.WallTime;
-UserTime   += RHS.UserTime;
+WallTime += RHS.WallTime;
+UserTime += RHS.UserTime;
 SystemTime += RHS.SystemTime;
-MemUsed+= RHS.MemUsed;
+MemUsed += RHS.MemUsed;
+InstructionsExecuted += RHS.InstructionsExecuted;
   }
   void operator-=(const TimeRecord ) {
-WallTime   -= RHS.WallTime;
-UserTime   -= RHS.UserTime;
+WallTime -= RHS.WallTime;
+