[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:
> tejohnson wrote:
> > 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.
> I see. That makes sense! I didn't understand the location meant by "computed below by the recursive caller." Your explanation "initial non-recursive invocation always has an empty Visited set" helps a lot. It means the immediate result of GetVCallVisibilityLevel may change, but the result for the initial invocation of the recursive call won't be changed. 
I've tried to clarify the comment accordingly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91676