[llvm] r244235 - This patch changes the interface to enable the shrink wrapping optimization.

Hal Finkel via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 12 02:32:28 PDT 2015


Hi Kit,

After some reflection, I'd like you to revert this patch. The design is not quite right, but more than that, it introduces a callback mechanism for this pass that is completely different from the mechanisms we use for any other feature.

First, the small problem with the design is that any target that provides its own callback function to control enabling this pass loses the ability to make use of the target-independent 'enable-shrink-wrap' cl::opt. As you point out in http://reviews.llvm.org/D11817, a target can directly reference the cl::opt (since it is not static), but this is not a desirable design, and each target must then replicate the logic for checking the option. The cl::opt is an implementation detail of the shrink wrapping pass, and should stay that way.

Moreover, you've moved part of the implementation of TargetPassConfig from Passes.cpp into ShrinkWrap.cpp, which is an unnecessary complication.

The logic in ShrinkWrap.cpp should read like this:

  switch (EnableShrinkWrapOpt) {
  case cl::BOU_TRUE:
    return true;
  case cl::BOU_UNSET:
    return TFI->enableShrinkWrapping();
  case cl::BOU_FALSE:
    return false;
  }

(or, since we have targetHandlesStackFrameRounding(), targetHandlesShrinkWrapping() would be good too).

The idea is that the target gets to override only the default behavior, but the command-line-option-handling logic only lives in one place. The target callback for this properly belongs in the target's TargetFrameLowering class along side the callbacks that actually implement the prologue/epilogue code. Please add a regular callback in TargetFrameLowering and use that for controlling the pass.

Thanks again,
Hal

----- Original Message -----
> From: "Kit Barton via llvm-commits" <llvm-commits at lists.llvm.org>
> To: llvm-commits at lists.llvm.org
> Sent: Thursday, August 6, 2015 1:02:53 PM
> Subject: [llvm] r244235 - This patch changes the interface to enable the shrink wrapping optimization.
> 
> Author: kbarton
> Date: Thu Aug  6 13:02:53 2015
> New Revision: 244235
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=244235&view=rev
> Log:
> This patch changes the interface to enable the shrink wrapping
> optimization.
> 
> It adds a new constructor, which takes a std::function predicate
> function that
> is run at the beginning of shrink wrapping to determine whether the
> optimization
> should run on the given machine function. The std::function can be
> overridden by
> each target, allowing target-specific decisions to be made on each
> machine
> function.
> 
> This is necessary for PowerPC, as the decision to run shrink wrapping
> is
> partially based on the ABI. Futhermore, this operates nicely with the
> GCC iFunc
> capability, which allows option overrides on a per-function basis.
> 
> Phabricator: http://reviews.llvm.org/D11421
> 
> Modified:
>     llvm/trunk/include/llvm/CodeGen/Passes.h
>     llvm/trunk/lib/CodeGen/Passes.cpp
>     llvm/trunk/lib/CodeGen/ShrinkWrap.cpp
> 
> Modified: llvm/trunk/include/llvm/CodeGen/Passes.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/Passes.h?rev=244235&r1=244234&r2=244235&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/Passes.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/Passes.h Thu Aug  6 13:02:53 2015
> @@ -120,9 +120,6 @@ protected:
>    /// Default setting for -enable-tail-merge on this target.
>    bool EnableTailMerge;
>  
> -  /// Default setting for -enable-shrink-wrap on this target.
> -  bool EnableShrinkWrap;
> -
>  public:
>    TargetPassConfig(TargetMachine *tm, PassManagerBase &pm);
>    // Dummy constructor.
> @@ -190,9 +187,6 @@ public:
>    /// Return true if the optimized regalloc pipeline is enabled.
>    bool getOptimizeRegAlloc() const;
>  
> -  /// Return true if shrink wrapping is enabled.
> -  bool getEnableShrinkWrap() const;
> -
>    /// Return true if the default global register allocator is in use
>    and
>    /// has not be overriden on the command line with '-regalloc=...'
>    bool usingDefaultRegAlloc() const;
> @@ -361,6 +355,14 @@ protected:
>    /// Add a pass to perform basic verification of the machine
>    function if
>    /// verification is enabled.
>    void addVerifyPass(const std::string &Banner);
> +
> +  /// Create an instance of ShrinkWrap using the runShrinkWrap
> predicate
> +  /// function.
> +  FunctionPass *createShrinkWrapPass();
> +
> +  /// Predicate function passed to a ShrinkWrap object to determine
> if shrink
> +  /// wrapping should be run on a MachineFunction.
> +  virtual bool runShrinkWrap(const MachineFunction &Fn) const;
>  };
>  } // namespace llvm
>  
> 
> Modified: llvm/trunk/lib/CodeGen/Passes.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/Passes.cpp?rev=244235&r1=244234&r2=244235&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/Passes.cpp (original)
> +++ llvm/trunk/lib/CodeGen/Passes.cpp Thu Aug  6 13:02:53 2015
> @@ -53,9 +53,6 @@ static cl::opt<bool> DisableMachineLICM(
>      cl::desc("Disable Machine LICM"));
>  static cl::opt<bool> DisableMachineCSE("disable-machine-cse",
>  cl::Hidden,
>      cl::desc("Disable Machine Common Subexpression Elimination"));
> -static cl::opt<cl::boolOrDefault>
> -    EnableShrinkWrapOpt("enable-shrink-wrap", cl::Hidden,
> -                        cl::desc("enable the shrink-wrapping
> pass"));
>  static cl::opt<cl::boolOrDefault> OptimizeRegAlloc(
>      "optimize-regalloc", cl::Hidden,
>      cl::desc("Enable optimized register allocation compilation
>      path."));
> @@ -218,7 +215,7 @@ TargetPassConfig::TargetPassConfig(Targe
>      : ImmutablePass(ID), PM(&pm), StartBefore(nullptr),
>      StartAfter(nullptr),
>        StopAfter(nullptr), Started(true), Stopped(false),
>        AddingMachinePasses(false), TM(tm), Impl(nullptr),
>        Initialized(false),
> -      DisableVerify(false), EnableTailMerge(true),
> EnableShrinkWrap(false) {
> +      DisableVerify(false), EnableTailMerge(true) {
>  
>    Impl = new PassConfigImpl();
>  
> @@ -540,8 +537,8 @@ void TargetPassConfig::addMachinePasses(
>    addPostRegAlloc();
>  
>    // Insert prolog/epilog code.  Eliminate abstract frame index
>    references...
> -  if (getEnableShrinkWrap())
> -    addPass(&ShrinkWrapID);
> +  if (getOptLevel() != CodeGenOpt::None)
> +    addPass(createShrinkWrapPass());
>    addPass(&PrologEpilogCodeInserterID);
>  
>    /// Add passes that optimize machine instructions after register
>    allocation.
> @@ -620,21 +617,6 @@ void TargetPassConfig::addMachineSSAOpti
>    addPass(&DeadMachineInstructionElimID);
>  }
>  
> -bool TargetPassConfig::getEnableShrinkWrap() const {
> -  switch (EnableShrinkWrapOpt) {
> -  case cl::BOU_UNSET:
> -    return EnableShrinkWrap && getOptLevel() != CodeGenOpt::None;
> -  // If EnableShrinkWrap is set, it takes precedence on whatever the
> -  // target sets. The rational is that we assume we want to test
> -  // something related to shrink-wrapping.
> -  case cl::BOU_TRUE:
> -    return true;
> -  case cl::BOU_FALSE:
> -    return false;
> -  }
> -  llvm_unreachable("Invalid shrink-wrapping state");
> -}
> -
>  //===---------------------------------------------------------------------===//
>  /// Register Allocation Pass Configuration
>  //===---------------------------------------------------------------------===//
> 
> Modified: llvm/trunk/lib/CodeGen/ShrinkWrap.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/ShrinkWrap.cpp?rev=244235&r1=244234&r2=244235&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/ShrinkWrap.cpp (original)
> +++ llvm/trunk/lib/CodeGen/ShrinkWrap.cpp Thu Aug  6 13:02:53 2015
> @@ -68,11 +68,16 @@
>  #include "llvm/Target/TargetInstrInfo.h"
>  // To access TargetInstrInfo.
>  #include "llvm/Target/TargetSubtargetInfo.h"
> +#include "llvm/Support/CommandLine.h"
>  
>  #define DEBUG_TYPE "shrink-wrap"
>  
>  using namespace llvm;
>  
> +static cl::opt<cl::boolOrDefault>
> +    EnableShrinkWrapOpt("enable-shrink-wrap", cl::Hidden,
> +                        cl::desc("enable the shrink-wrapping
> pass"));
> +
>  STATISTIC(NumFunc, "Number of functions");
>  STATISTIC(NumCandidates, "Number of shrink-wrapping candidates");
>  STATISTIC(NumCandidatesDropped,
> @@ -154,6 +159,11 @@ public:
>    ShrinkWrap() : MachineFunctionPass(ID) {
>      initializeShrinkWrapPass(*PassRegistry::getPassRegistry());
>    }
> +
> +  ShrinkWrap(std::function<bool(const MachineFunction &)> Ftor) :
> +      MachineFunctionPass(ID), PredicateFtor(Ftor) {
> +    initializeShrinkWrapPass(*PassRegistry::getPassRegistry());
> +  }
>  
>    void getAnalysisUsage(AnalysisUsage &AU) const override {
>      AU.setPreservesAll();
> @@ -171,6 +181,15 @@ public:
>    /// \brief Perform the shrink-wrapping analysis and update
>    /// the MachineFrameInfo attached to \p MF with the results.
>    bool runOnMachineFunction(MachineFunction &MF) override;
> +
> +private:
> +  /// \brief Predicate function to determine if shrink wrapping
> should run.
> +  ///
> +  /// This function will be run at the beginning of shrink wrapping
> and
> +  /// determine whether shrink wrapping should run on the given
> MachineFunction.
> +  /// \param[in] MF The MachineFunction to run shrink wrapping on.
> +  /// \return true if shrink wrapping should be run, false
> otherwise.
> +  std::function<bool(const MachineFunction &MF)> PredicateFtor;
>  };
>  } // End anonymous namespace.
>  
> @@ -301,8 +320,12 @@ void ShrinkWrap::updateSaveRestorePoints
>  }
>  
>  bool ShrinkWrap::runOnMachineFunction(MachineFunction &MF) {
> -  if (MF.empty())
> +  if (PredicateFtor && !PredicateFtor(MF))
>      return false;
> +
> +  if (MF.empty() || skipOptnoneFunction(*MF.getFunction()))
> +    return false;
> +
>    DEBUG(dbgs() << "**** Analysing " << MF.getName() << '\n');
>  
>    init(MF);
> @@ -386,3 +409,26 @@ bool ShrinkWrap::runOnMachineFunction(Ma
>    ++NumCandidates;
>    return false;
>  }
> +
> +/// If EnableShrinkWrap is set run shrink wrapping on the given
> Machine
> +/// Function. Otherwise, shrink wrapping is disabled.
> +/// This function can be overridden in each target-specific
> TargetPassConfig
> +/// class to allow different predicate logic for each target.
> +bool TargetPassConfig::runShrinkWrap(const MachineFunction &Fn)
> const {
> +  switch (EnableShrinkWrapOpt) {
> +  case cl::BOU_TRUE:
> +    return true;
> +  case cl::BOU_UNSET:
> +  case cl::BOU_FALSE:
> +    return false;
> +  }
> +  llvm_unreachable("Invalid shrink-wrapping state");
> +}
> +
> +/// Create a ShrinkWrap FunctionPass using the runShrinkWrap
> predicate
> +/// function.
> +FunctionPass *TargetPassConfig::createShrinkWrapPass() {
> +  std::function<bool(const MachineFunction &Fn)> Ftor =
> +    std::bind(&TargetPassConfig::runShrinkWrap, this,
> std::placeholders::_1);
> +  return new ShrinkWrap(Ftor);
> +}
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory


More information about the llvm-commits mailing list