[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:78
+    // lookup will fail if Name exceeds the size cap.
+    return vmap.lookup(capLocalValueName(Name));
+  }
----------------
serge-sans-paille wrote:
> hasyimibhar wrote:
> > mehdi_amini wrote:
> > > The fact that the table lookups would automatically use this but not the insertions looks like a red flag to me. If this is an invariant of the table then it should be private the table and reflected by the API consistently.
> > I moved NonGlobalValueMaxNameSize into ValueSymbolTable, and made it responsible for doing the capping. It looks like the updated name will be propagated back to the Value class (since createValueName is called by Value::setName), so it should be fine to move it.
> > 
> > I also added isLocal argument to ValueSymbolTable::lookup, because it seems that it's also used to look up global value in LLParser, so there must be a way to differentiate between global and local value name lookup.
> 
> > I also added isLocal argument to ValueSymbolTable::lookup, because it seems that it's also used to look up global value in LLParser, so there must be a way to differentiate between global and local value name lookup.
> 
> That's why I suggested to create a type adaptor: a type,  that derives from ValueSymbolTable
> 
> 
Ok let me try to do that. Looking at the code, I'm assuming only the `Module` will use symbol table for looking up global names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102707