Bug 137155 – Native inlining should resolve internal references

• Component: JavaScriptCore
• Status: NEW

[Matthew Mirman]

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.

[Matthew Mirman]

Created attachment 238729 [details]
The patch for LLVM for adding the pass

Not yet for review, for archival purposes.

[Matthew Mirman]

Created attachment 238730 [details]
The patch for Clang to add the attribute

Not yet for review.  For archival purposes.

[Matthew Mirman]

Created attachment 238732 [details]
The patch for LLVM for adding the pass

Still not yet for review.  I forgot a file.

[Filip Pizlo]

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.

[Filip Pizlo]

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?

[Filip Pizlo]

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.

[Matthew Mirman]

Created attachment 239756 [details]
The patch for LLVM for adding the pass with the file. 

Changed the name of the assertion.

[Filip Pizlo]

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.

[Matthew Mirman]

Created attachment 239757 [details]
The patch for Clang to add the attribute

Renames the clang assertion to something reasonable.

[Filip Pizlo]

Comment on attachment 239757 [details]
The patch for Clang to add the attribute

This part LGTM.

[Matthew Mirman]

(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.

[Filip Pizlo]

(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?

[Matthew Mirman]

(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]"

[Filip Pizlo]

(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!