When native inlining in the FTL imports LLVM IR to inline, it doesn't change references to internal variables and functions which can no longer be resolved during compilation. Currently there actually isn't a way to do this without modifying LLVM and clang, so here's a modification to LLVM and clang that makes this, among other things possible.
Created attachment 238729 [details] The patch for LLVM for adding the pass Not yet for review, for archival purposes.
Created attachment 238730 [details] The patch for Clang to add the attribute Not yet for review. For archival purposes.
Created attachment 238732 [details] The patch for LLVM for adding the pass Still not yet for review. I forgot a file.
Comment on attachment 238732 [details] The patch for LLVM for adding the pass View in context: https://bugs.webkit.org/attachment.cgi?id=238732&action=review > lib/Transforms/Scalar/LiftInternalCalls.cpp:63 > + const GlobalValue *Pt = dyn_cast<GlobalValue>(Op); > + if (Pt && GlobalValue::isLocalLinkage(Pt->getLinkage())) > + Refs.insert(Pt->getName()); I think this misses constexpr's that transitively reference GlobalValues.
Comment on attachment 238732 [details] The patch for LLVM for adding the pass View in context: https://bugs.webkit.org/attachment.cgi?id=238732&action=review > lib/Transforms/Scalar/LiftInternalCalls.cpp:55 > + StringSet<> Refs; Why can't this just say StringSet instead of StringSet<>? Also, wouldn't it be better to use a SmallPtrSet of GlobalValues?
Comment on attachment 238732 [details] The patch for LLVM for adding the pass View in context: https://bugs.webkit.org/attachment.cgi?id=238732&action=review > lib/Transforms/Scalar/LiftInternalCalls.cpp:83 > + Constant **TableVals = new Constant*[Refs.size()]; Seems like you should use some built-in vector type here, like std::vector or SmallVector. > lib/Transforms/Scalar/LiftInternalCalls.cpp:106 > + MDNode *MetaTableRefValue = > + MDNode::get(M.getContext(), ArrayRef<Value *>(Loc)); I don't think we want these to be MDNodes. MDNodes aren't required to survive compilation.
Created attachment 239756 [details] The patch for LLVM for adding the pass with the file. Changed the name of the assertion.
Comment on attachment 239756 [details] The patch for LLVM for adding the pass with the file. View in context: https://bugs.webkit.org/attachment.cgi?id=239756&action=review > lib/Transforms/Scalar/PrepareForJITInlining.cpp:124 > + // now simply add a metadata entry which points to this value! > + // metadata will be removed when linking so it won't polute the namespace. > + > + std::string MetaTableRefName = RefName + NewGlobSuffix; > + NamedMDNode *MetaTableRef = > + M.getOrInsertNamedMetadata(MetaTableRefName); So, this creates a symbol table that disappears before the JIT gets to run? I don't think that's what you want.
Created attachment 239757 [details] The patch for Clang to add the attribute Renames the clang assertion to something reasonable.
Comment on attachment 239757 [details] The patch for Clang to add the attribute This part LGTM.
(In reply to comment #8) > (From update of attachment 239756 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239756&action=review > > > lib/Transforms/Scalar/PrepareForJITInlining.cpp:124 > > + // now simply add a metadata entry which points to this value! > > + // metadata will be removed when linking so it won't polute the namespace. > > + > > + std::string MetaTableRefName = RefName + NewGlobSuffix; > > + NamedMDNode *MetaTableRef = > > + M.getOrInsertNamedMetadata(MetaTableRefName); > > So, this creates a symbol table that disappears before the JIT gets to run? I don't think that's what you want. This is in face what I want. The array of referenced internal symbol locations doesn't disappear at runtime, only the metadata which maps the names of these functions to their locations in that array. The metadata doesn't get erased from the llvm ir which gets stored in ".llvmir.MODULENAME" which is loaded at by the JIT. The JIT then gets the locations in the array from the llvm ir rather than the runtime values. This way, the names are easier to encode, the hashtable's mechanics are already implemented via llvm's find metadata by name mechanism, and the namespace still isn't polluted anymore.
(In reply to comment #11) > (In reply to comment #8) > > (From update of attachment 239756 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=239756&action=review > > > > > lib/Transforms/Scalar/PrepareForJITInlining.cpp:124 > > > + // now simply add a metadata entry which points to this value! > > > + // metadata will be removed when linking so it won't polute the namespace. > > > + > > > + std::string MetaTableRefName = RefName + NewGlobSuffix; > > > + NamedMDNode *MetaTableRef = > > > + M.getOrInsertNamedMetadata(MetaTableRefName); > > > > So, this creates a symbol table that disappears before the JIT gets to run? I don't think that's what you want. > > This is in face what I want. The array of referenced internal symbol locations doesn't disappear at runtime, only the metadata which maps the names of these functions to their locations in that array. The metadata doesn't get erased from the llvm ir which gets stored in ".llvmir.MODULENAME" which is loaded at by the JIT. The JIT then gets the locations in the array from the llvm ir rather than the runtime values. This way, the names are easier to encode, the hashtable's mechanics are already implemented via llvm's find metadata by name mechanism, and the namespace still isn't polluted anymore. How are you going to use this to get the pointer to the value using the name?
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #8) > > > (From update of attachment 239756 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=239756&action=review > > > > > > > lib/Transforms/Scalar/PrepareForJITInlining.cpp:124 > > > > + // now simply add a metadata entry which points to this value! > > > > + // metadata will be removed when linking so it won't polute the namespace. > > > > + > > > > + std::string MetaTableRefName = RefName + NewGlobSuffix; > > > > + NamedMDNode *MetaTableRef = > > > > + M.getOrInsertNamedMetadata(MetaTableRefName); > > > > > > So, this creates a symbol table that disappears before the JIT gets to run? I don't think that's what you want. > > > > This is in face what I want. The array of referenced internal symbol locations doesn't disappear at runtime, only the metadata which maps the names of these functions to their locations in that array. The metadata doesn't get erased from the llvm ir which gets stored in ".llvmir.MODULENAME" which is loaded at by the JIT. The JIT then gets the locations in the array from the llvm ir rather than the runtime values. This way, the names are easier to encode, the hashtable's mechanics are already implemented via llvm's find metadata by name mechanism, and the namespace still isn't polluted anymore. > > How are you going to use this to get the pointer to the value using the name? in the jit: check if the function (L) is inlineable by searching for "nameoffunction.jitinlineable". If so, link the module's llvm ir pointed to by "nameoffunction.jitinlineable" into the current module (if its not already loaded). For the function L, for each variable access, check if there is a metadata entry in the current module named "REFNAME.MODULENAME". If not move on. If so, change it to ".internalRefsTable.MODULENAME[REFNAME.MODULENAME metadata's data]"
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (In reply to comment #8) > > > > (From update of attachment 239756 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=239756&action=review > > > > > > > > > lib/Transforms/Scalar/PrepareForJITInlining.cpp:124 > > > > > + // now simply add a metadata entry which points to this value! > > > > > + // metadata will be removed when linking so it won't polute the namespace. > > > > > + > > > > > + std::string MetaTableRefName = RefName + NewGlobSuffix; > > > > > + NamedMDNode *MetaTableRef = > > > > > + M.getOrInsertNamedMetadata(MetaTableRefName); > > > > > > > > So, this creates a symbol table that disappears before the JIT gets to run? I don't think that's what you want. > > > > > > This is in face what I want. The array of referenced internal symbol locations doesn't disappear at runtime, only the metadata which maps the names of these functions to their locations in that array. The metadata doesn't get erased from the llvm ir which gets stored in ".llvmir.MODULENAME" which is loaded at by the JIT. The JIT then gets the locations in the array from the llvm ir rather than the runtime values. This way, the names are easier to encode, the hashtable's mechanics are already implemented via llvm's find metadata by name mechanism, and the namespace still isn't polluted anymore. > > > > How are you going to use this to get the pointer to the value using the name? > > in the jit: > > check if the function (L) is inlineable by searching for "nameoffunction.jitinlineable". > If so, link the module's llvm ir pointed to by "nameoffunction.jitinlineable" into the current module (if its not already loaded). > For the function L, for each variable access, check if there is a metadata entry in the current module named "REFNAME.MODULENAME". If not move on. If so, change it to ".internalRefsTable.MODULENAME[REFNAME.MODULENAME metadata's data]" Oh, I see now!