[LLVMdev] Removing AvailableExternal values in GlobalDCE (was Re: RFC: ThinLTO Impementation Plan)
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Jun 8 14:31:20 PDT 2015
bcc:llvmdev, +llvm-commits
> On 2015 Jun 8, at 14:13, Teresa Johnson <tejohnson at google.com> wrote:
>
> New patches attached. Passes llvm/clang regression tests. PTAL
> Thanks!
> Teresa
>
> On Mon, Jun 8, 2015 at 1:39 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
>>
>>> On 2015 Jun 8, at 13:35, Eric Christopher <echristo at gmail.com> wrote:
>>>
>>>
>>>
>>> On Mon, Jun 8, 2015 at 1:33 PM Reid Kleckner <rnk at google.com> wrote:
>>> On Mon, Jun 8, 2015 at 1:08 PM, Eric Christopher <echristo at gmail.com> wrote:
>>> I'd rather not have it be an llvm option at all. Just construct a different set of passes...
>>>
>>> This would also solve the problem of needing multiple sets of options to be passed to the builder. It'd be a bit of a change (i.e. having clang do the pass setup), but I think it'd be worth it to have clang be able to explicitly say which passes it wants.
>>>
>>> It's also a major change and would probably need to be discussed more widely so for now I guess this is sorta ok.
>>>
>>> I think for now the bool on PassManagerBuilder is a good way to make progress. Eventually, we may want to customize the LTO compilation phase optimization more fully (use lower inlining threshold, defer vectorization to link time), but we can cross that bridge when we get there.
>>>
>>> Precisely. In case it wasn't clear I was saying this.
>>
>> This SGTM too.
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
> <patch.ElimAE_clang2.txt><patch.ElimAE_llvm2.txt>
LLVM patch LGTM, with a few bikesheds below.
> Index: include/llvm/InitializePasses.h
> ===================================================================
> --- include/llvm/InitializePasses.h (revision 237590)
> +++ include/llvm/InitializePasses.h (working copy)
> @@ -130,6 +130,7 @@ void initializeSanitizerCoverageModulePass(PassReg
> void initializeDataFlowSanitizerPass(PassRegistry&);
> void initializeScalarizerPass(PassRegistry&);
> void initializeEarlyCSELegacyPassPass(PassRegistry &);
> +void initializeElimAvailExternPass(PassRegistry&);
> void initializeExpandISelPseudosPass(PassRegistry&);
> void initializeFunctionAttrsPass(PassRegistry&);
> void initializeGCMachineCodeAnalysisPass(PassRegistry&);
> Index: include/llvm/Transforms/IPO.h
> ===================================================================
> --- include/llvm/Transforms/IPO.h (revision 237590)
> +++ include/llvm/Transforms/IPO.h (working copy)
> @@ -71,6 +71,12 @@ ModulePass *createGlobalOptimizerPass();
> ModulePass *createGlobalDCEPass();
>
> //===----------------------------------------------------------------------===//
> +/// createElimAvailExternPass - This transform is designed to eliminate
This is old-style commenting. Please skip the name of the function in
the doxygen comment.
> +/// available external globals (functions or global variables)
> +///
> +ModulePass *createElimAvailExternPass();
This name isn't clear to me. "elimavailextern" is fine as command-line
shorthand, but I think the function should be more explicit, something
like `createEliminateAvailableExternallyPass()`.
> +
> +//===----------------------------------------------------------------------===//
> /// createGVExtractionPass - If deleteFn is true, this pass deletes
> /// the specified global values. Otherwise, it deletes as much of the module as
> /// possible, except for the global values specified.
> Index: include/llvm/Transforms/IPO/PassManagerBuilder.h
> ===================================================================
> --- include/llvm/Transforms/IPO/PassManagerBuilder.h (revision 237590)
> +++ include/llvm/Transforms/IPO/PassManagerBuilder.h (working copy)
> @@ -121,6 +121,7 @@ class PassManagerBuilder {
> bool VerifyInput;
> bool VerifyOutput;
> bool MergeFunctions;
> + bool PrepareForLTO;
>
> private:
> /// ExtensionList - This is list of all of the extensions that are registered.
> Index: lib/Transforms/IPO/CMakeLists.txt
> ===================================================================
> --- lib/Transforms/IPO/CMakeLists.txt (revision 237590)
> +++ lib/Transforms/IPO/CMakeLists.txt (working copy)
> @@ -3,6 +3,7 @@ add_llvm_library(LLVMipo
> BarrierNoopPass.cpp
> ConstantMerge.cpp
> DeadArgumentElimination.cpp
> + ElimAvailExtern.cpp
> ExtractGV.cpp
> FunctionAttrs.cpp
> GlobalDCE.cpp
> Index: lib/Transforms/IPO/ElimAvailExtern.cpp
> ===================================================================
> --- lib/Transforms/IPO/ElimAvailExtern.cpp (revision 0)
> +++ lib/Transforms/IPO/ElimAvailExtern.cpp (working copy)
> @@ -0,0 +1,93 @@
> +//===-- ElimAvailExtern.cpp - DCE unreachable internal functions ----------------===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This transform is designed to eliminate available external global
> +// definitions from the program, turning them into declarations.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#include "llvm/Transforms/IPO.h"
> +#include "llvm/ADT/Statistic.h"
> +#include "llvm/IR/Constants.h"
> +#include "llvm/IR/Instructions.h"
> +#include "llvm/IR/Module.h"
> +#include "llvm/Transforms/Utils/CtorUtils.h"
> +#include "llvm/Transforms/Utils/GlobalStatus.h"
> +#include "llvm/Pass.h"
> +using namespace llvm;
> +
> +#define DEBUG_TYPE "elimavailextern"
Would this be better with hyphens ("elim-avail-extern")? I don't really
have an opinion of which is better, just asking.
> +
> +STATISTIC(NumAliases , "Number of global aliases removed");
> +STATISTIC(NumFunctions, "Number of functions removed");
> +STATISTIC(NumVariables, "Number of global variables removed");
> +
> +namespace {
> + struct ElimAvailExtern : public ModulePass {
I think the class name (like the `create*()` function) should be spelled
out more explicilty: `EliminateAvailableExternally`.
> + static char ID; // Pass identification, replacement for typeid
> + ElimAvailExtern() : ModulePass(ID) {
> + initializeElimAvailExternPass(*PassRegistry::getPassRegistry());
> + }
> +
> + // run - Do the ElimAvailExtern pass on the specified module, optionally
> + // updating the specified callgraph to reflect the changes.
> + //
> + bool runOnModule(Module &M) override;
> + };
> +}
> +
> +char ElimAvailExtern::ID = 0;
> +INITIALIZE_PASS(ElimAvailExtern, "elimavailextern",
If add hyphens to the `DEBUG_TYPE`, add hyphens here too.
> + "Eliminate Available External Globals", false, false)
s/External/Externally/
> +
> +ModulePass *llvm::createElimAvailExternPass() { return new ElimAvailExtern(); }
> +
> +bool ElimAvailExtern::runOnModule(Module &M) {
> + bool Changed = false;
> +
> + // Drop initializers of available externally global variables.
> + for (Module::global_iterator I = M.global_begin(), E = M.global_end();
> + I != E; ++I) {
> + if (!I->hasAvailableExternallyLinkage())
> + continue;
> + if (I->hasInitializer()) {
> + Constant *Init = I->getInitializer();
> + I->setInitializer(nullptr);
> + if (isSafeToDestroyConstant(Init))
> + Init->destroyConstant();
> + }
> + I->removeDeadConstantUsers();
> + I->setLinkage(GlobalValue::ExternalLinkage);
> + NumVariables++;
> + }
> +
> + // Drop the bodies of available externally functions.
> + for (Module::iterator I = M.begin(), E = M.end(); I != E; ++I) {
> + if (!I->hasAvailableExternallyLinkage())
> + continue;
> + if (!I->isDeclaration())
> + // This will set the linkage to external
> + I->deleteBody();
> + I->removeDeadConstantUsers();
> + NumFunctions++;
> + }
> +
> + // Drop targets of available externally aliases.
> + for (Module::alias_iterator I = M.alias_begin(), E = M.alias_end(); I != E;
> + ++I) {
> + if (!I->hasAvailableExternallyLinkage())
> + continue;
> + I->setAliasee(nullptr);
> + I->removeDeadConstantUsers();
> + I->setLinkage(GlobalValue::ExternalLinkage);
> + NumAliases++;
> + }
> +
> + return Changed;
> +}
> Index: lib/Transforms/IPO/PassManagerBuilder.cpp
> ===================================================================
> --- lib/Transforms/IPO/PassManagerBuilder.cpp (revision 237590)
> +++ lib/Transforms/IPO/PassManagerBuilder.cpp (working copy)
> @@ -106,6 +106,7 @@ PassManagerBuilder::PassManagerBuilder() {
> VerifyInput = false;
> VerifyOutput = false;
> MergeFunctions = false;
> + PrepareForLTO = false;
> }
>
> PassManagerBuilder::~PassManagerBuilder() {
> @@ -407,6 +408,17 @@ void PassManagerBuilder::populateModulePassManager
> // GlobalOpt already deletes dead functions and globals, at -O2 try a
> // late pass of GlobalDCE. It is capable of deleting dead cycles.
> if (OptLevel > 1) {
> + if (!PrepareForLTO) {
> + // Remove avail extern fns and globals definitions if we aren't
> + // compiling an object file for later LTO. For LTO we want to preserve
> + // these so they are eligible for inlining at link-time. Note if they
> + // are unreferenced they will be removed by GlobalDCE below, so
> + // this only impacts referenced available externally globals.
> + // Eventually they will be suppressed during codegen, but eliminating
> + // here enables more opportunity for GlobalDCE as it may make
> + // globals referenced by available external functions dead.
> + MPM.add(createElimAvailExternPass());
> + }
> MPM.add(createGlobalDCEPass()); // Remove dead fns and globals.
> MPM.add(createConstantMergePass()); // Merge dup global constants
> }
>
More information about the llvm-dev
mailing list