[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:
> > 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?


Repository:
  rL LLVM

https://reviews.llvm.org/D51330