[PATCH] CodeGen: Add IntegerDivisionPass

[Tom Stellard]

On Wed, Sep 24, 2014 at 11:19:47AM -0700, Michael Ilseman wrote:
> +bool IntegerDivision::runOnFunction(Function &F) {
> +
> +  if (TM)
> +    TLI = TM->getSubtargetImpl()->getTargetLowering();
> +
> +  for (Function::iterator BBI = F.begin(), BBE = F.end(); BBI != BBE; ++BBI) {
> +       BasicBlock *BB = BBI;
> +    for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE; ++II) {
> +        Instruction *I = II;
> +        if (visit(*I)) {
> +          BBI = F.begin();
> +          break;
> +        }
> +    }
> +  }
> +
> +  return false;
> +}
> 
> You should have a bool for whether the code changed, and return that[1]. It’s probably worth building up a worklist of basic blocks up front, to avoid the repeated visiting of already-handled blocks. Also, we know that many of the blocks that are generated from division-expansion would not themselves have rem or div in them.
> 
> 
> +bool IntegerDivision::shouldExpandDivRem(const BinaryOperator &I) {
> +  return TLI && TLI->shouldExpandDivRemInIR(I);
> +}
> 
> You could do the TLI-is-not-null check in runOnFunction, which is a good place to bail early. Thereafter you can have an assert. I don’t know who would be using this pass normally, though.
> 
> Otherwise the patch LGTM. What is your use case of wanting a full-fledged pass for this? If you have other codegen-prepare-like things to do to the IR, it seems suboptimal to do a pass over every instruction just for div/rem, at least from a low-latency compilation time perspective.
> 

No specific reason for using a pass.  Do you think it would be better to add
this to CodegenPrepare instead?

-Tom

> 
> [1] http://llvm.org/docs/WritingAnLLVMPass.html#the-runonfunction-method
> 
> 
> 
> 
> 
> 
> On Sep 24, 2014, at 10:06 AM, Tom Stellard <tom at stellard.net> wrote:
> 
> > On Tue, Sep 23, 2014 at 01:57:23PM -0700, Michael Ilseman wrote:
> >> 
> >>> On Sep 23, 2014, at 12:48 PM, Tom Stellard <thomas.stellard at amd.com> wrote:
> >>> 
> >>> This pass will expand unsupported div/rem instructions based on
> >>> the result of the newly added TargetLowering::shouldExpandDivRemInIR().
> >>> ---
> >>> 
> >>> A future commit will enable this pass for the R600 backend and add tests.
> >>> 
> >>> include/llvm/CodeGen/Passes.h        |   6 ++
> >>> include/llvm/InitializePasses.h      |   1 +
> >>> include/llvm/Target/TargetLowering.h |   6 ++
> >>> lib/CodeGen/CMakeLists.txt           |   1 +
> >>> lib/CodeGen/IntegerDivisionPass.cpp  | 124 +++++++++++++++++++++++++++++++++++
> >>> 5 files changed, 138 insertions(+)
> >>> create mode 100644 lib/CodeGen/IntegerDivisionPass.cpp
> >>> 
> >>> diff --git a/include/llvm/CodeGen/Passes.h b/include/llvm/CodeGen/Passes.h
> >>> index 31fba89..3fedd5a 100644
> >>> --- a/include/llvm/CodeGen/Passes.h
> >>> +++ b/include/llvm/CodeGen/Passes.h
> >>> @@ -376,6 +376,9 @@ namespace llvm {
> >>>  /// load-linked/store-conditional loops.
> >>>  extern char &AtomicExpandID;
> >>> 
> >>> +  /// Lowers unsupported integer division.
> >>> +  extern char &IntegerDivisionID;
> >>> +
> >>>  /// MachineLoopInfo - This pass is a loop analysis pass.
> >>>  extern char &MachineLoopInfoID;
> >>> 
> >>> @@ -598,6 +601,9 @@ namespace llvm {
> >>> 
> >>>  /// createJumpInstrTables - This pass creates jump-instruction tables.
> >>>  ModulePass *createJumpInstrTablesPass();
> >>> +
> >>> +  /// Lower unsupported integer division
> >>> +  FunctionPass *createIntegerDivisionPass(const TargetMachine *TM);
> >>> } // End llvm namespace
> >>> 
> >>> /// This initializer registers TargetMachine constructor, so the pass being
> >>> diff --git a/include/llvm/InitializePasses.h b/include/llvm/InitializePasses.h
> >>> index 2964798..99f7ab5 100644
> >>> --- a/include/llvm/InitializePasses.h
> >>> +++ b/include/llvm/InitializePasses.h
> >>> @@ -146,6 +146,7 @@ void initializeInlineCostAnalysisPass(PassRegistry&);
> >>> void initializeInstCombinerPass(PassRegistry&);
> >>> void initializeInstCountPass(PassRegistry&);
> >>> void initializeInstNamerPass(PassRegistry&);
> >>> +void initializeIntegerDivisionPass(PassRegistry&);
> >>> void initializeInternalizePassPass(PassRegistry&);
> >>> void initializeIntervalPartitionPass(PassRegistry&);
> >>> void initializeJumpInstrTableInfoPass(PassRegistry&);
> >>> diff --git a/include/llvm/Target/TargetLowering.h b/include/llvm/Target/TargetLowering.h
> >>> index 0a72225..245df33 100644
> >>> --- a/include/llvm/Target/TargetLowering.h
> >>> +++ b/include/llvm/Target/TargetLowering.h
> >>> @@ -1000,6 +1000,12 @@ public:
> >>>    return false;
> >>>  }
> >>> 
> >>> +  /// Returns true if the instruction should be expanded by the IR-level
> >>> +  /// IntegerDivision pass.
> >>> +  virtual bool shouldExpandDivRemInIR(const BinaryOperator &I) const {
> >>> +    return false;
> >>> +  }
> >>> +
> >>>  //===--------------------------------------------------------------------===//
> >>>  // TargetLowering Configuration Methods - These methods should be invoked by
> >>>  // the derived class constructor to configure this object for the target.
> >>> diff --git a/lib/CodeGen/CMakeLists.txt b/lib/CodeGen/CMakeLists.txt
> >>> index 0e06ed0..d002f47 100644
> >>> --- a/lib/CodeGen/CMakeLists.txt
> >>> +++ b/lib/CodeGen/CMakeLists.txt
> >>> @@ -25,6 +25,7 @@ add_llvm_library(LLVMCodeGen
> >>>  GlobalMerge.cpp
> >>>  IfConversion.cpp
> >>>  InlineSpiller.cpp
> >>> +  IntegerDivisionPass.cpp
> >>>  InterferenceCache.cpp
> >>>  IntrinsicLowering.cpp
> >>>  JumpInstrTables.cpp
> >>> diff --git a/lib/CodeGen/IntegerDivisionPass.cpp b/lib/CodeGen/IntegerDivisionPass.cpp
> >>> new file mode 100644
> >>> index 0000000..b198a1f
> >>> --- /dev/null
> >>> +++ b/lib/CodeGen/IntegerDivisionPass.cpp
> >>> @@ -0,0 +1,124 @@
> >>> +//===-- IntegerDivisionPass.cpp - Expand div/mod instructions -------------===//
> >>> +//
> >>> +//                     The LLVM Compiler Infrastructure
> >>> +//
> >>> +// This file is distributed under the University of Illinois Open Source
> >>> +// License. See LICENSE.TXT for details.
> >>> +//
> >>> +//===----------------------------------------------------------------------===//
> >>> +//
> >>> +/// \file
> >>> +//===----------------------------------------------------------------------===//
> >>> +
> >>> +#include "llvm/CodeGen/Passes.h"
> >>> +#include "llvm/IR/IRBuilder.h"
> >>> +#include "llvm/IR/InstVisitor.h"
> >>> +#include "llvm/Target/TargetLowering.h"
> >>> +#include "llvm/Target/TargetMachine.h"
> >>> +#include "llvm/Target/TargetSubtargetInfo.h"
> >>> +#include "llvm/Transforms/Utils/IntegerDivision.h"
> >>> +using namespace llvm;
> >>> +
> >>> +#define DEBUG_TYPE "integer-division"
> >>> +
> >>> +namespace {
> >>> +
> >>> +class IntegerDivision : public FunctionPass,
> >>> +                        public InstVisitor<IntegerDivision, bool> {
> >>> +
> >>> +  const TargetMachine *TM;
> >>> +  const TargetLowering *TLI;
> >>> +
> >>> +  std::vector<BinaryOperator *> Divs;
> >>> +  std::vector<BinaryOperator *> Rems;
> >>> +
> >> 
> >> As far as I can tell from the patch, these vectors are not used. What is their intent?
> >> 
> > 
> > These are leftover from a previous version of the patch.  Here is an updated
> > patch with them removed.
> > 
> > -Tom
> > <0001-CodeGen-Add-IntegerDivisionPass.patch>
>