john.brawn added inline comments.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:542
+ N = Decls.size();
+ }
+
----------------
rjmccall wrote:
> dexonsmith wrote:
> > john.brawn wrote:
> > > rjmccall wrote:
> > > > john.brawn wrote:
> > > > > rjmccall wrote:
> > > > > > This is going to fire on every single ordinary lookup that finds
> > > > > > multiple declarations, right? I haven't fully internalized the
> > > > > > issue you're solving here, but this is a very performance-sensitive
> > > > > > path in the compiler; there's a reason this code is written to very
> > > > > > carefully only do extra work when we've detected in the loop below
> > > > > > that we're in a hidden-declarations situation. Is there any way we
> > > > > > can restore that basic structure?
> > > > > Test4 in the added tests is an example of why we can't wait until
> > > > > after the main loop. The `using A::X` in namespace D is eliminated by
> > > > > the UniqueResult check, so the check for a declaration being hidden
> > > > > can only see the using declarations in namespace C. We also can't do
> > > > > it in the loop itself, as then we can't handle Test5: at the time we
> > > > > process the `using A::X` in namespace D it looks like it may cause
> > > > > ambiguity, but it's later hidden by the `using B::X` in the same
> > > > > namespace which we haven't yet processed.
> > > > >
> > > > > I have adjusted it though so the nested loop and erasing of decls
> > > > > only happens when we both have things that hide and things that can
> > > > > be hidden. Doing some quick testing of compiling SemaOpenMP.cpp (the
> > > > > largest file in clang), LookupResult::resolveKind is called 75318
> > > > > times, of which 75283 calls have HideTags=true, of which 56 meet this
> > > > > condition, i.e. 0.07%.
> > > > Okay, I can see why you need to not mix tag-hiding with the removal of
> > > > duplicates. However, I think you can maintain the current structure by
> > > > delaying the actual removal of declarations until after the main loop;
> > > > have the loop build up a set of indices to remove instead. (Also, you
> > > > can keep this set as a bitset instead of a `std::set<unsigned>`.)
> > > >
> > > > It's a shame that the hiding algorithm has to check every other
> > > > declaration in the lookup in case they're from different scopes. I
> > > > guess to avoid that we'd have to do the filtering immediately when we
> > > > collect the declarations from a particular DC.
> > > I think that delaying the removal until after the main loop would just
> > > complicate things, as then in the main loop we would have to check each
> > > index to see if it's something we're going to later remove. I can adjust
> > > it to do the erasing more like it's done in the main loop though, i.e.
> > > move the erased element to the end and decrement N, so the call to
> > > Decls.truncate will remove it. We can't use bitset though, as that takes
> > > the size of the bitset (which in this case would be the number of decls)
> > > as a template parameter.
> > llvm::BitVector should work for this.
> Why would the main loop need to check indices to see if it's something we're
> going to remove? You just need to check whether a tag is hidden before you
> add it to `UniqueTypes`.
That seems to be the same thing I said, but phrased in a different way? It's
not entirely clear to me what exactly you want the code to look like. It sound
like you'd want it to be something like
```
llvm::BitVector HiddenDecls(N);
if (HideTags) {
// Add hidden decls to HiddenDecls
}
while (I < N) {
if (HiddenDecls[I]) // Ignore hidden decls
continue;
// Existing code in main loop
}
for (I : HiddenDecls)
// remove this decl
```
I don't see how this would be better. Why delay the removal when we can just do
it right away?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154503/new/
https://reviews.llvm.org/D154503
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
