rjmccall added inline comments.
================ Comment at: clang/lib/Sema/SemaLookup.cpp:542 + N = Decls.size(); + } + ---------------- 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`. 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
