https://github.com/jansvoboda11 closed
https://github.com/llvm/llvm-project/pull/66962
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
jansvoboda11 wrote:
@sam-mccall Do you have any additional feedback? You might want to check how
this PR and #66966 impact performance for you.
https://github.com/llvm/llvm-project/pull/66962
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
jansvoboda11 wrote:
> Thanks for iterating! I find the current implementation much clearer.
Thanks for your patience!
> The only thing I might quibble about is the "child" vs. "parent" terminology
> you changed: I think it's fairly ambiguous either way, because the node is
> the "child" from
https://github.com/benlangmuir approved this pull request.
Thanks for iterating! I find the current implementation much clearer.
The only thing I might quibble about is the "child" vs. "parent" terminology
you changed: I think it's fairly ambiguous either way, because the node is the
"child"
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/66962
>From 61e8961cde95e9e8ce8cea3efd6aa52273f430e9 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Wed, 20 Sep 2023 16:50:48 -0700
Subject: [PATCH 1/6] [clang][modules] Remove preloaded SLocEntries from PCM
@@ -1986,13 +1984,15 @@ bool SourceManager::isInTheSameTranslationUnitImpl(
if (isLoadedFileID(LOffs.first) != isLoadedFileID(ROffs.first))
return false;
- // If both are loaded from different AST files.
if (isLoadedFileID(LOffs.first) &&
@@ -1986,13 +1984,15 @@ bool SourceManager::isInTheSameTranslationUnitImpl(
if (isLoadedFileID(LOffs.first) != isLoadedFileID(ROffs.first))
return false;
- // If both are loaded from different AST files.
if (isLoadedFileID(LOffs.first) &&
https://github.com/jansvoboda11 edited
https://github.com/llvm/llvm-project/pull/66962
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -458,14 +458,12 @@ SourceManager::AllocateLoadedSLocEntries(unsigned
NumSLocEntries,
CurrentLoadedOffset - TotalSize < NextLocalOffset) {
return std::make_pair(0, 0);
}
-
- unsigned NewTableSize = LoadedSLocEntryTable.size() + NumSLocEntries;
-
@@ -458,14 +458,12 @@ SourceManager::AllocateLoadedSLocEntries(unsigned
NumSLocEntries,
CurrentLoadedOffset - TotalSize < NextLocalOffset) {
return std::make_pair(0, 0);
}
-
- unsigned NewTableSize = LoadedSLocEntryTable.size() + NumSLocEntries;
-
https://github.com/benlangmuir edited
https://github.com/llvm/llvm-project/pull/66962
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -458,14 +458,12 @@ SourceManager::AllocateLoadedSLocEntries(unsigned
NumSLocEntries,
CurrentLoadedOffset - TotalSize < NextLocalOffset) {
return std::make_pair(0, 0);
}
-
- unsigned NewTableSize = LoadedSLocEntryTable.size() + NumSLocEntries;
-
@@ -458,14 +458,12 @@ SourceManager::AllocateLoadedSLocEntries(unsigned
NumSLocEntries,
CurrentLoadedOffset - TotalSize < NextLocalOffset) {
return std::make_pair(0, 0);
}
-
- unsigned NewTableSize = LoadedSLocEntryTable.size() + NumSLocEntries;
-
@@ -1986,13 +1984,15 @@ bool SourceManager::isInTheSameTranslationUnitImpl(
if (isLoadedFileID(LOffs.first) != isLoadedFileID(ROffs.first))
return false;
- // If both are loaded from different AST files.
if (isLoadedFileID(LOffs.first) &&
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/66962
>From 61e8961cde95e9e8ce8cea3efd6aa52273f430e9 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Wed, 20 Sep 2023 16:50:48 -0700
Subject: [PATCH 1/5] [clang][modules] Remove preloaded SLocEntries from PCM
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/66962
>From 61e8961cde95e9e8ce8cea3efd6aa52273f430e9 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Wed, 20 Sep 2023 16:50:48 -0700
Subject: [PATCH 1/4] [clang][modules] Remove preloaded SLocEntries from PCM
@@ -701,6 +701,10 @@ class SourceManager : public RefCountedBase
{
/// use (-ID - 2).
SmallVector LoadedSLocEntryTable;
+ /// For each allocation in LoadedSLocEntryTable, we keep the new size. This
+ /// can be used to determine whether two FileIDs come from the same
https://github.com/jansvoboda11 edited
https://github.com/llvm/llvm-project/pull/66962
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/jansvoboda11 edited
https://github.com/llvm/llvm-project/pull/66962
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -701,6 +701,10 @@ class SourceManager : public RefCountedBase
{
/// use (-ID - 2).
SmallVector LoadedSLocEntryTable;
+ /// For each allocation in LoadedSLocEntryTable, we keep the new size. This
+ /// can be used to determine whether two FileIDs come from the same
@@ -458,11 +458,14 @@ SourceManager::AllocateLoadedSLocEntries(unsigned
NumSLocEntries,
CurrentLoadedOffset - TotalSize < NextLocalOffset) {
return std::make_pair(0, 0);
}
- LoadedSLocEntryTable.resize(LoadedSLocEntryTable.size() + NumSLocEntries);
-
@@ -701,6 +701,10 @@ class SourceManager : public RefCountedBase
{
/// use (-ID - 2).
SmallVector LoadedSLocEntryTable;
+ /// For each allocation in LoadedSLocEntryTable, we keep the new size. This
+ /// can be used to determine whether two FileIDs come from the same
@@ -2082,6 +2082,11 @@ std::pair
SourceManager::isInTheSameTranslationUnit(
if (LOffs.first == ROffs.first)
return std::make_pair(true, LOffs.second < ROffs.second);
+ // Built-ins are not considered part of any TU.
benlangmuir wrote:
Yes, this
@@ -2082,6 +2082,11 @@ std::pair
SourceManager::isInTheSameTranslationUnit(
if (LOffs.first == ROffs.first)
return std::make_pair(true, LOffs.second < ROffs.second);
+ // Built-ins are not considered part of any TU.
jansvoboda11 wrote:
This comment
@@ -458,11 +458,14 @@ SourceManager::AllocateLoadedSLocEntries(unsigned
NumSLocEntries,
CurrentLoadedOffset - TotalSize < NextLocalOffset) {
return std::make_pair(0, 0);
}
- LoadedSLocEntryTable.resize(LoadedSLocEntryTable.size() + NumSLocEntries);
-
@@ -458,11 +458,14 @@ SourceManager::AllocateLoadedSLocEntries(unsigned
NumSLocEntries,
CurrentLoadedOffset - TotalSize < NextLocalOffset) {
return std::make_pair(0, 0);
}
- LoadedSLocEntryTable.resize(LoadedSLocEntryTable.size() + NumSLocEntries);
-
@@ -701,6 +701,10 @@ class SourceManager : public RefCountedBase
{
/// use (-ID - 2).
SmallVector LoadedSLocEntryTable;
+ /// For each allocation in LoadedSLocEntryTable, we keep the new size. This
+ /// can be used to determine whether two FileIDs come from the same
@@ -2082,6 +2082,11 @@ std::pair
SourceManager::isInTheSameTranslationUnit(
if (LOffs.first == ROffs.first)
return std::make_pair(true, LOffs.second < ROffs.second);
+ // Built-ins are not considered part of any TU.
benlangmuir wrote:
Does this have
jansvoboda11 wrote:
> > That said, isBeforeInTranslationUnit is slow already, and I don't think
> > it's hot in clang, moreso in tools like clang-tidy whose performance is
> > less-critical. So let's make it right, and then optimize it again if
> > problems arise.
>
> It may not be a hot
tahonermann wrote:
> That said, isBeforeInTranslationUnit is slow already, and I don't think it's
> hot in clang, moreso in tools like clang-tidy whose performance is
> less-critical. So let's make it right, and then optimize it again if problems
> arise.
It may not be a hot function in
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/66962
>From 61e8961cde95e9e8ce8cea3efd6aa52273f430e9 Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Wed, 20 Sep 2023 16:50:48 -0700
Subject: [PATCH 1/3] [clang][modules] Remove preloaded SLocEntries from PCM
jansvoboda11 wrote:
I don't understand why the tie-breaking code calls the previously visited
`FileID` a parent. We're walking **up** the include/expansion tree, so I think
it should be called child instead. LMK if I'm misunderstanding.
https://github.com/llvm/llvm-project/pull/66962
jansvoboda11 wrote:
Turns out some clients are calling `isBeforeInTranslationUnit()` before
checking if both `SourceLocations` are indeed in the same TU. I left behind a
FIXME to call `llvm_unreachable()`, but for now, I just compare the `FileIDs`
to keep things working.
jansvoboda11 wrote:
Since I moved up the special-casing of built-ins buffer up in my last commit,
it was happening before we got chance to walk up from `FileID(3)` to
`FileID(2)`, meaning it wasn't kicking in. I think we need to sink the
special-casing into `isInTheSameTranslationUnit()` and
jansvoboda11 wrote:
I got around investigating
"SemaCXX/warn-unsafe-buffer-usage-fixits-parm-span.cpp" and found out it uses
this pattern:
```
%clang_cc1 ... -include %s %s
```
This means `FileID(1)` is the main input file `%s` and `FileID(2)` is the
built-ins buffer that includes `%s` as
sam-mccall wrote:
Ripping out this feature sounds great, as does fixing the non-translation of
builtin buffer locations.
My only concern is that this will make isBeforeInTranslationUnit more
expensive. A loc being in one of these special "" is the rare case,
and the code recognizing them
benlangmuir wrote:
Thanks for laying it out. I also find this easier to understand in your latest
changes because the comments in `isBeforeInTranslationUnit` lay out the
intended ordering without relying on `isInTheSameTranslationUnit` returning
`false` for builtin buffers, which is the part
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/66962
>From 1e36c68229a98ffbdb61ea652077bc3356fc177f Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Wed, 20 Sep 2023 16:50:48 -0700
Subject: [PATCH 1/4] [clang][modules] Remove preloaded SLocEntries from PCM
jansvoboda11 wrote:
> > I changed SourceManager::isInTheSameTranslationUnit() to check for that
> > case explicitly instead of relying on the fact/bug that built-ins buffers
> > happened to be assigned an untranslated include SourceLocation.
>
> What is the translated location you get here?
benlangmuir wrote:
> I changed SourceManager::isInTheSameTranslationUnit() to check for that case
> explicitly instead of relying on the fact/bug that built-ins buffers happened
> to be assigned an untranslated include SourceLocation.
What is the translated location you get here? It's hard
https://github.com/jansvoboda11 resolved
https://github.com/llvm/llvm-project/pull/66962
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/66962
>From 1e36c68229a98ffbdb61ea652077bc3356fc177f Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Wed, 20 Sep 2023 16:50:48 -0700
Subject: [PATCH 1/3] [clang][modules] Remove preloaded SLocEntries from PCM
https://github.com/jansvoboda11 updated
https://github.com/llvm/llvm-project/pull/66962
>From 1e36c68229a98ffbdb61ea652077bc3356fc177f Mon Sep 17 00:00:00 2001
From: Jan Svoboda
Date: Wed, 20 Sep 2023 16:50:48 -0700
Subject: [PATCH 1/2] [clang][modules] Remove preloaded SLocEntries from PCM
jansvoboda11 wrote:
Ok, the comment below seems to suggests that the built-ins buffers are not
considered to be part of any TU:
```c++
bool SourceManager::isBeforeInTranslationUnit(SourceLocation LHS,
SourceLocation RHS) const {
// ...
if
jansvoboda11 wrote:
The "Index/annotate-module.m" test fails with this patch. For run line 29, the
previous input is:
```
Punctuation: "#" [1:1 - 1:2] inclusion directive=Module/Sub2.h
(/Users/Jan/Code/upstream-llvm/clang/test/Index/../Modules/Inputs/Module.framework/Headers/Sub2.h)
https://github.com/benlangmuir approved this pull request.
LGTM other than my nit about the comment
https://github.com/llvm/llvm-project/pull/66962
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
jansvoboda11 wrote:
> > in my experience, it's not actually referenced in most modules
>
> Can you clarify what you looked at here? I assume you checked non-scanner
> modules as well?
I took a typical Xcode project and chose one TU that happens to transitively
depend on 37 modules. I built
https://github.com/benlangmuir edited
https://github.com/llvm/llvm-project/pull/66962
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/benlangmuir commented:
> in my experience, it's not actually referenced in most modules
Can you clarify what you looked at here? I assume you checked non-scanner
modules as well?
https://github.com/llvm/llvm-project/pull/66962
___
@@ -524,13 +524,7 @@ enum ASTRecordTypes {
/// of source-location information.
SOURCE_LOCATION_OFFSETS = 14,
- /// Record code for the set of source location entries
- /// that need to be preloaded by the AST reader.
- ///
- /// This set contains the source location
llvmbot wrote:
@llvm/pr-subscribers-clang-modules
Changes
This commit removes the list of SLocEntry offsets to preload eagerly from PCM
files. Commit introducing this functionality (258ae54a) doesn't clarify why
this would be more performant than the lazy approach used regularly.
https://github.com/jansvoboda11 created
https://github.com/llvm/llvm-project/pull/66962
This commit removes the list of SLocEntry offsets to preload eagerly from PCM
files. Commit introducing this functionality (258ae54a) doesn't clarify why
this would be more performant than the lazy
52 matches
Mail list logo
