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