[PATCH] D51330: [ThinLTO] Efficiency fix for writing type id records in per-module indexes

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:744
+    GlobalValue::GUID,
+    TinyPtrVector<const std::pair<const std::string, TypeIdSummary> *>>;
+
----------------
pcc wrote:
> tejohnson wrote:
> > pcc wrote:
> > > tejohnson wrote:
> > > > tejohnson wrote:
> > > > > pcc wrote:
> > > > > > tejohnson wrote:
> > > > > > > @pcc - do you remember why you used a vector as the map value type? Presumably there should be a 1-1 ratio between the GUIDs computed from type ids and the type id map entry, but is this to handle the potential for GUID conflicts?
> > > > > > Yes, it was for guid conflicts.
> > > > > These should be rare and from what I can tell, if we only track one type id per conflicting GUID (since we will also have the type id string to disambiguate), the worst that will happen is that we wouldn't get a resolution and it would result in conservative behavior. Is that correct? Or do we need to track them all for correct behavior?
> > > > > 
> > > > > I realized when looking at changing this that currently the assembly writer will not behave well in the presence of conflicts, since the slot ID for the summary entry is assigned by GUID.
> > > > > 
> > > > > If we can get by with it, the implementation would be simpler if we simply tracked one (and like I said, disambiguate via the std::string saved in the value).
> > > > My new patch does away with the vector of summaries for each GUID. The interface to get a summary and the clients are changed to handle this - which I believe is conservative. If my interpretation of this as being conservative is incorrect please let me know.
> > > There's no conservative behaviour for type id resolutions, they need to be correct or we will miscompile (e.g. by forming a CFI check incorrectly).
> > Ok. I assumed that the return of the Unsat kind at the start of LowerTypeTestsModule::importTypeId when there is not a type id summary in the index would lead to conservative checking in that scenario. What situation is that handling?
> Unsat is for handling the case where there is a call with no possible targets. For example a vcall where all vtables of that type have been dead stripped. In that case we compile the check so that it always fails. The reason why we return unsat when there are no type id summaries is that the absence of any vtables with a type id would lead to no summary for that type id being created.
I see, thanks for the explanation.

I suppose I should first fix the assembly printing for type ids so that the slot is based on the string and not on its GUID. Should be straightforward - I'll fix that first separately and update this patch to go to a vector of type summaries per GUID in the map afterwards.


Repository:
  rL LLVM

https://reviews.llvm.org/D51330