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

[Hasyimi Bahrudin via Phabricator via llvm-commits]

hasyimibhar added inline comments.


================
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;
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102707