[PATCH] D34063: [ThinLTO] YAML traits for module summaries

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:537
+  // Maps Oid GUID to the stringified value name (for dissassembly)
+  std::map<GlobalValue::GUID, std::string> OidToValueName;
+
----------------
ncharlie wrote:
> tejohnson wrote:
> > ncharlie wrote:
> > > mehdi_amini wrote:
> > > > tejohnson wrote:
> > > > > ncharlie wrote:
> > > > > > mehdi_amini wrote:
> > > > > > > ncharlie wrote:
> > > > > > > > mehdi_amini wrote:
> > > > > > > > > Do we want this on all the time? Is this a fundamental property of the Index?
> > > > > > > > > It isn't clear to me if it isn't just a debugging tool that map to the IR when available right now?
> > > > > > > > How would you recommend making this an optional debugging option? Would a constructor argument work?
> > > > > > > Is this information in the index in the bitcode? 
> > > > > > > If not then it can be reconstructed at dump time.
> > > > > > The reason I'm storing the map in the index is because I only have access to the ValueName when the bitcode is being parsed in ModuleSummaryIndexBitcodeReader (see BitcodeReader.cpp:4715 in this patch). Also, when the YAML is being generated, I'm only given a pointer to a ModuleSummaryIndex, which is why the map is in that class.
> > > > > The value name is available on the GlobalValue (Value::getName()), as is the GUID (getGUID()). So you can just walk the Module's list of global_objects() (which gives you all the functions and global variables) and the list of aliases(), and compute the mapping. 
> > > > > 
> > > > > That being said, I am not familiar enough with the YAML support to know how to get an additional object (either the Module, or a mapping pre-computed by the client or a wrapper function), passed down to where you need it. pcc may have some insight.
> > > > > Also, when the YAML is being generated, I'm only given a pointer to a ModuleSummaryIndex, which is why the map is in that class.
> > > > 
> > > > Then just modify the YAML to get an optional  `Module *` as well? If you get it then you have names available, otherwise not.
> > > > Since it wouldn't roundtrip the YAML, I'd display it as a comment next to the GUID.
> > > > The value name is available on the GlobalValue (Value::getName()), as is the GUID (getGUID()). So you can just walk the Module's list of global_objects()
> > > 
> > > > Then just modify the YAML to get an optional  Module * as well? 
> > > 
> > > OK, I'll look into to this. I didn't realize there was already a map that stored the names in the Module class, so now I really want to get rid of my duplicate.
> > It isn't really a map, but rather the global values are available from the Module, and as I mentioned in my comment (just above Mehdi's), the names and guids are available on the global values.
> > Then just modify the YAML to get an optional `Module *` as well?
> 
> After looking into this, I can see two options for doing this.
> 
>   # Create a struct that holds both the Module * and the ModuleSummaryIndex, and output the new struct. I went part way down this path today, and I definitely think it's possible to do this, but it might result in some slightly odd code in some places - especially when reading in YAML (since it may not always be entirely obvious why a ModuleSummaryIndex and Module * have to be tracked together).
>   # Add a `Module *` or `getModule` function to the ModuleSummaryIndex - I think this option is the cleanest and simplest implementation, despite adding a bit more information to the ModuleSummaryIndex class.
> 
> If there's some way to get a Module from a ModuleID or ModuleHash, that could work as well, but I'm not sure if it's possible.
> 
> Let me know which one you guys think is the best.
> 
I'm not sure we're all on the same track.

Names are never part of the summaries IIRC. So when you only work to/from YAML you don't have any names. Which is why I mentioned that the names in the YAML can only be comments.

I don't expect any change to include/llvm/IR/ModuleSummaryIndex.h ; it should be fully handled in the routine that dumps to YAML: it should take an optional Module. The first it can do is build a map from GUID -> Name for every symbol in the module. The YAML dump takes the summary index and this map and process from here.



https://reviews.llvm.org/D34063