[PATCH] D102707: Fix non-global-value-max-name-size not considered by LLParser

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini accepted this revision.
mehdi_amini added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/IR/ValueSymbolTable.h:74
   /// Lookup a named Value.
-  Value *lookup(StringRef Name) const { return vmap.lookup(Name); }
+  virtual Value *lookup(StringRef Name) const;
 
----------------
hasyimibhar wrote:
> mehdi_amini wrote:
> > hasyimibhar wrote:
> > > mehdi_amini wrote:
> > > > serge-sans-paille wrote:
> > > > > @mehdi_amini are we fine with making a few methods here virtual? The approach proposed by @hasyimibhar is elegant, but I'm worried about the performance impact.
> > > > > 
> > > > > An alternative would be to have something like (pseudo code)
> > > > > 
> > > > > ```
> > > > > class LocalValueSymbolTable {
> > > > >     ValueSymbolTable table;
> > > > >     public:
> > > > >       using ValueSymbolTable::size;  // nothing to change for that one
> > > > >       Value *lookup(StringRef Name) const { // cap then forward to table }
> > > > >     private:
> > > > >       ValueName *createValueName(StringRef Name, Value *V); { // cap then forward to table }
> > > > > ```
> > > > >       
> > > > >       
> > > > +1: let's not make virtual if we can have an adapter class.
> > > > 
> > > > Another way to do it is by refactoring the base class for the common methods, and two derived class that implement the specific methods for each case. You only need virtual for the derived method if you actually need to use this class polymorphically (do you?).
> > > > 
> > > So I tried to do it with adapter class, but it gets messy because the class is used polymorphically in `Value.cpp` by `getSymTab` function. With adapter class, `Function::getValueSymbolTable` will return the adapter class, while `Module::getValueSymbolTable` will return the base class. So what ends up happening is inside `getSymTab`, I had to unwrap the adapter class back to the base class, and then in `Value::setNameImpl`, I had to check if `this` is a local value, and if so, wrap the symbol table back with the adapter class.
> > > 
> > > My suggestion to avoid all this mess while still maintaining performance is to just add `isLocal` private variable to differentiate between local and global value symbol table. This avoids cost of `virtual` and doesn't require changing the API of `lookup`.
> > > 
> > > Also, maybe it's better to just make it explicit and have the constructor accept a `maxNameSize` argument? For global, we can set this to `-1` to indicate no limit.
> > OK, but right now this new member is only used for lookups and not insertions, which makes the invariant non-local the class I believe. Can you make sure `makeUniqueName` has the matching logic so that this is all self contained in the ValueSymbolTable?
> The name is already capped in `createValueName` (in `ValueSymbolTable.cpp`), which I believe is the insertion part. `makeUniqueName` happens after the cap, so you can still have name longer than the limit if there's duplicate, but I think that's the correct behavior.
Ah my bad, I missed this somehow!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102707