[llvm-commits] Make library interface pass for review

[David Chisnall]

On 9 May 2010, at 19:23, Chris Lattner wrote:

> 
> On May 8, 2010, at 1:46 PM, David Chisnall wrote:
> 
>> Hi,
>> 
>> Does anyone object if I commit the attached diff?  It's a work-in-progress pass that prepares the bitcode used to compile a shared library for use by the inliner.  After running the pass, the resulting bitcode can be linked into code that links against the library and used for inlining and any other related optimisations.
>> 
>> In theory, it strips out anything that depends on globals that are not exposed outside of the module and marks everything as available_externally.  In practice, there are almost certainly some cases that I've missed (reports welcome)...
>> 
>> David
> 
> A few requests:
> 
> Please document this in Passes.html

Done.

> +++ lib/Transforms/IPO/MakeRuntimeLibraryInterface.cpp	(revision 0)
> @@ -0,0 +1,201 @@
> +#include "llvm/Pass.h"
> +#include "llvm/Function.h"
> +#include "llvm/Module.h"
> +#include "llvm/LLVMContext.h"
> +#include "llvm/Instructions.h"
> +#include "llvm/Constants.h"
> +#include "llvm/GlobalVariable.h"
> 
> You need a file header commend, please prune out the redundant includes you don't need.

Fixed / added.

> +namespace
> +{
> +  class MakeRuntimeLibraryInterface : public ModulePass 
> +  {
> 
> Please put the { on the same line, conforming to the predominate style.

Fixed.

> +  for (Module::iterator I=M.begin(), E=M.end() ;
> +      I!=E ; ++I) {
> 
> Please use whitespace like the rest of llvm :).  You don't need braces if the for loop contains a single statement.

Fixed

> Please remove ProcessGlobals / IteratorMethod until and when they are needed.

Ooops.  'when they are needed' is in the past - I thought I'd removed them already.

> 
> +  switch (V->getLinkage()) {
> +    case GlobalValue::ExternalLinkage:
> +    case GlobalValue::DLLExportLinkage:
> +    case GlobalValue::ExternalWeakLinkage:
> +    case GlobalValue::CommonLinkage:
> +      V->setLinkage(GlobalValue::AvailableExternallyLinkage);
> +      return true;
> +    case GlobalValue::AvailableExternallyLinkage:
> +      // Already done
> +    case GlobalValue::WeakAnyLinkage:
> +    case GlobalValue::WeakODRLinkage:
> +    case GlobalValue::LinkOnceAnyLinkage:
> +    case GlobalValue::LinkOnceODRLinkage:
> +      // Copy link-once and weak stuff
> +    case GlobalValue::AppendingLinkage:
> +    case GlobalValue::InternalLinkage:
> +    case GlobalValue::PrivateLinkage:
> +    case GlobalValue::LinkerPrivateLinkage:
> +    case GlobalValue::DLLImportLinkage:
> +      // Ignore internal symbols here.
> +      return false;
> 
> Use predicates (like hasLocalLinkage) instead of enumerating all of this stuff.  

Okay.

> Also, you should be able to handle linkonce_odr and weak_odr.  

Probably the _any versions too... those should be visible externally, I think.  Can we just turn all of these into 

> Since you have several *different* places where your reasoning about the same linkage types, you should probably write your own predicates as a static inline function in your file, instead of duplicating the logic all over.

I currently have three switch block, each testing for a different one of these conditions.  I think, in fact, that some of this logic was wrong (I said it was a work in progress).  The new version now only has two cases:

1) is something visible externally.
2) Is something that is visible externally something that should not be initialized here.

I've factored 1) out into an inline function, because it's used in two places, but I've left 2) inline.

> Please resend this for another iteration after you incorporate these changes,

Attached.  I'm still not 100% convinced that the logic is correct, especially with respect to handling linkonce linkage types.  Should they be turned into something else and their initialisers removed, or should they be left as is?  I'm now leaving them as-is, but I'm not convinced that this is the right thing to do.  

David

--
This email complies with ISO 3103
-------------- next part --------------
A non-text attachment was scrubbed...
Name: MakeRuntimeLibraryInterface.diff
Type: application/octet-stream
Size: 11175 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100510/7e2841c9/attachment.obj>