[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.


================
Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302
+  // has no effect on the min visibility computed below by the recursive caller.
+  if (!Visited.insert(RD).second)
+    return llvm::GlobalObject::VCallVisibilityTranslationUnit;
+
----------------
wmi wrote:
> If a CXXRecordDecl is visited twice, the visibility returned in the second visit could be larger than necessary. Will it change the final result? If it will, can we cache the visibility result got in the first visit instead of returning the max value? 
The recursive callsites compute the std::min of the current TypeVis and the one returned by the recursive call. So returning the max guarantees that there is no effect on the current TypeVis. Let me know if the comment can be clarified (that's what I meant by "so that it has no effect on the min visibility computed below ...". Note that the initial non-recursive invocation always has an empty Visited set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91676