[NFC] Loop Versioning for LICM code clean up

[eastig]

Summary

Clean up of the code of Loop Versioning for LICM. See comments to the changes below.

[eastig]

lib/Transforms/Scalar/LoopVersioningLICM.cpp

161 ↗

(On Diff #74219)

TargetLibraryInfo is not used in the code.

169 ↗

(On Diff #74219)

Removed unused fields or made them function scoped instead of class scoped.

176 ↗

(On Diff #74219)

Made internal data private.

305 ↗

(On Diff #74219)

It's strange checking. It was questioned in https://reviews.llvm.org/D9151. An answer was:

Actually this is a pre-condition in LICM’s “promoteLoopAccessesToScalars” where it expects all pointers in alias should have same type.
To confirm same behaviour we added this check.

Yes, type checking is needed for AliasSet when all pointers must alias. This is done by LICM. In case of AliasSet whose pointers may alias we can have aliased pointers of different types, e.g. char * and char ** which might be aliased and they have different types.

510 ↗

(On Diff #74219)

Converted to be function scoped.

512 ↗

(On Diff #74219)

Converted to be function scoped.

515 ↗

(On Diff #74219)

Converted to be function scoped.

515–516 ↗

(On Diff #74219)

Removed as unused.

522 ↗

(On Diff #74219)

Removed as unused.

524 ↗

(On Diff #74219)

This is not safe. Replaced with unique_ptr.

567 ↗

(On Diff #74219)

Removed as unused.

[eastig]

Any objections to commit these changes?

[sbaranga]

lib/Transforms/Scalar/LoopVersioningLICM.cpp

305 ↗

(On Diff #74219)

I agree, this does look strange (and it might not be a NFC), I think either Ashutosh or Adam should comment on this.

524 ↗

(On Diff #74219)

Why is this not safe? It looks like if we don't use use unique_ptr we end up with less memory usage (unique_ptr won't free the memory until LoopVersioningLICMs destructor gets called or until we run on another loop).

[eastig]

lib/Transforms/Scalar/LoopVersioningLICM.cpp

305 ↗

(On Diff #74219)

I'll revert for this review.

524 ↗

(On Diff #74219)

All separate new and delete are unsafe. Yes, you are right it's hold till destruction of a LoopVersioningLICM object. I'll fix this.

[eastig]

Updated according to Silviu's comments

[ashutosh.nema]

Changes looks OK, except one minor formatting comment.

lib/Transforms/Scalar/LoopVersioningLICM.cpp

166 ↗

(On Diff #74441)

Please correct formatting, can use clang-format here.

305 ↗

(On Diff #74219)

Type check is required for LICM, it expects pointers in the alias set should have the same type.

[eastig]

lib/Transforms/Scalar/LoopVersioningLICM.cpp

305 ↗

(On Diff #74219)

Yes, it is required but LICM does it for alias sets with must-alias pointers. Here an alias set with may-alias pointers is checked. The goal of the LoopVersioningLICM pass is to create a version of a loop where all alias sets with may-alias pointers are converted either into sets with must-alias pointers or single-element sets. The checking is similar to what LICM does but it has different meaning. It restricts may-alias pointers used in a loop to have the same type. It excludes a case when 'char *' pointer can be aliased with everything. For example the following loop is not versioned because of this checking:

void f(int *a, int *b, char *c, int n) {
  for (int i = 0; i < n; ++i) {
    a[i] = b[i] + *c;
    *c = (char)a[i];
  }
}

In this case an alias set consists of 'a', 'b' and 'c'. We can calculate bounds of all pointers used in the loop. So we can create RT checks and move memory accesses to 'c' out of the loop.

[eastig]

Done code formatting.

[eastig]

I assume the changes are approved. I am committing them.

[ashutosh.nema]

LGTM.

[sbaranga]

lib/Transforms/Scalar/LoopVersioningLICM.cpp

522 ↗

(On Diff #74491)

This only releases the memory when running on a new loop. So it wouldn't make things better. The solution also looks a bit over-engineered.

524 ↗

(On Diff #74219)

I still don't understand why new/delete would be unsafe.

[sbaranga]

LGTM

lib/Transforms/Scalar/LoopVersioningLICM.cpp

522 ↗

(On Diff #74491)

Ok, Evgeny just explained to me that this would release the memory when it goes out of scope. I think it's fine now.

llvm/trunk/lib/Transforms/Scalar/LoopVersioningLICM.cpp

 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/LoopUtils.h"
 #include "llvm/Transforms/Utils/LoopVersioning.h"
 #include "llvm/Transforms/Utils/ValueMapper.h"
 
 #define DEBUG_TYPE "loop-versioning-licm"
-static const char* LICMVersioningMetaData =
-    "llvm.loop.licm_versioning.disable";
+static const char *LICMVersioningMetaData = "llvm.loop.licm_versioning.disable";
 
 using namespace llvm;
 
 /// Threshold minimum allowed percentage for possible
 /// invariant instructions in a loop.
 static cl::opt<float>
     LVInvarThreshold("licm-versioning-invariant-threshold",
                      cl::desc("LoopVersioningLICM's minimum allowed percentage"
void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
     AU.addRequired<AAResultsWrapperPass>();
     AU.addRequired<DominatorTreeWrapperPass>();
     AU.addRequiredID(LCSSAID);
     AU.addRequired<LoopAccessLegacyAnalysis>();
     AU.addRequired<LoopInfoWrapperPass>();
     AU.addRequiredID(LoopSimplifyID);
     AU.addRequired<ScalarEvolutionWrapperPass>();
-    AU.addRequired<TargetLibraryInfoWrapperPass>();
     AU.addPreserved<AAResultsWrapperPass>();
     AU.addPreserved<GlobalsAAWrapperPass>();
   }
 
   LoopVersioningLICM()
-      : LoopPass(ID), AA(nullptr), SE(nullptr), LI(nullptr), DT(nullptr),
-        TLI(nullptr), LAA(nullptr), LAI(nullptr), Changed(false),
-        Preheader(nullptr), CurLoop(nullptr), CurAST(nullptr),
-        LoopDepthThreshold(LVLoopDepthThreshold),
+      : LoopPass(ID), AA(nullptr), SE(nullptr), LAA(nullptr), LAI(nullptr),
+        CurLoop(nullptr), LoopDepthThreshold(LVLoopDepthThreshold),
         InvariantThreshold(LVInvarThreshold), LoadAndStoreCounter(0),
         InvariantCounter(0), IsReadOnlyLoop(true) {
     initializeLoopVersioningLICMPass(*PassRegistry::getPassRegistry());
   }
+  StringRef getPassName() const override { return "Loop Versioning for LICM"; }
 
-  AliasAnalysis *AA;         // Current AliasAnalysis information
-  ScalarEvolution *SE;       // Current ScalarEvolution
-  LoopInfo *LI;              // Current LoopInfo
-  DominatorTree *DT;         // Dominator Tree for the current Loop.
-  TargetLibraryInfo *TLI;    // TargetLibraryInfo for constant folding.
-  LoopAccessLegacyAnalysis *LAA;   // Current LoopAccessAnalysis
-  const LoopAccessInfo *LAI; // Current Loop's LoopAccessInfo
+  void reset() {
+    AA = nullptr;
+    SE = nullptr;
+    LAA = nullptr;
+    CurLoop = nullptr;
+    LoadAndStoreCounter = 0;
+    InvariantCounter = 0;
+    IsReadOnlyLoop = true;
+    CurAST.reset();
+  }
+
+  class AutoResetter {
+  public:
+    AutoResetter(LoopVersioningLICM &LVLICM) : LVLICM(LVLICM) {}
+    ~AutoResetter() { LVLICM.reset(); }
+
+  private:
+    LoopVersioningLICM &LVLICM;
+  };
+
+private:
+  AliasAnalysis *AA;             // Current AliasAnalysis information
+  ScalarEvolution *SE;           // Current ScalarEvolution
+  LoopAccessLegacyAnalysis *LAA; // Current LoopAccessAnalysis
+  const LoopAccessInfo *LAI;     // Current Loop's LoopAccessInfo
 
-  bool Changed;            // Set to true when we change anything.
-  BasicBlock *Preheader;   // The preheader block of the current loop.
-  Loop *CurLoop;           // The current loop we are working on.
-  AliasSetTracker *CurAST; // AliasSet information for the current loop.
-  ValueToValueMap Strides;
+  Loop *CurLoop; // The current loop we are working on.
+  std::unique_ptr<AliasSetTracker>
+      CurAST; // AliasSet information for the current loop.
 
   unsigned LoopDepthThreshold;  // Maximum loop nest threshold
   float InvariantThreshold;     // Minimum invariant threshold
   unsigned LoadAndStoreCounter; // Counter to track num of load & store
   unsigned InvariantCounter;    // Counter to track num of invariant
   bool IsReadOnlyLoop;          // Read only loop marker.
 
   bool isLegalForVersioning();
   bool legalLoopStructure();
   bool legalLoopInstructions();
   bool legalLoopMemoryAccesses();
   bool isLoopAlreadyVisited();
   void setNoAliasToLoop(Loop *);
   bool instructionSafeForVersioning(Instruction *);
-  StringRef getPassName() const override { return "Loop Versioning"; }
 };
 }
 
 /// \brief Check loop structure and confirms it's good for LoopVersioningLICM.
 bool LoopVersioningLICM::legalLoopStructure() {
   // Loop must have a preheader, if not return false.
   if (!CurLoop->getLoopPreheader()) {
     DEBUG(dbgs() << "    loop preheader is missing\n");
for (auto &Inst : *Block) {
           LLVMContext::MD_alias_scope,
           MDNode::concatenate(Inst.getMetadata(LLVMContext::MD_alias_scope),
                               MDNode::get(Inst.getContext(), Scopes)));
     }
   }
 }
 
 bool LoopVersioningLICM::runOnLoop(Loop *L, LPPassManager &LPM) {
+  // This will automatically release all resources hold by the current
+  // LoopVersioningLICM object.
+  AutoResetter Resetter(*this);
+
   if (skipLoop(L))
     return false;
-  Changed = false;
   // Get Analysis information.
-  LI = &getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
   AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
   SE = &getAnalysis<ScalarEvolutionWrapperPass>().getSE();
-  DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
-  TLI = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
   LAA = &getAnalysis<LoopAccessLegacyAnalysis>();
   LAI = nullptr;
   // Set Current Loop
   CurLoop = L;
-  // Get the preheader block.
-  Preheader = L->getLoopPreheader();
-  // Initial allocation
-  CurAST = new AliasSetTracker(*AA);
+  CurAST.reset(new AliasSetTracker(*AA));
 
   // Loop over the body of this loop, construct AST.
+  LoopInfo *LI = &getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
   for (auto *Block : L->getBlocks()) {
     if (LI->getLoopFor(Block) == L) // Ignore blocks in subloop.
       CurAST->add(*Block);          // Incorporate the specified basic block
   }
+
+  bool Changed = false;
+
   // Check feasiblity of LoopVersioningLICM.
   // If versioning found to be feasible and beneficial then proceed
   // else simply return, by cleaning up memory.
   if (isLegalForVersioning()) {
     // Do loop versioning.
     // Create memcheck for memory accessed inside loop.
     // Clone original loop, and set blocks properly.
+    DominatorTree *DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
     LoopVersioning LVer(*LAI, CurLoop, LI, DT, SE, true);
     LVer.versionLoop();
     // Set Loop Versioning metaData for original loop.
     addStringMetadataToLoop(LVer.getNonVersionedLoop(), LICMVersioningMetaData);
     // Set Loop Versioning metaData for version loop.
     addStringMetadataToLoop(LVer.getVersionedLoop(), LICMVersioningMetaData);
     // Set "llvm.mem.parallel_loop_access" metaData to versioned loop.
     addStringMetadataToLoop(LVer.getVersionedLoop(),
                             "llvm.mem.parallel_loop_access");
     // Update version loop with aggressive aliasing assumption.
     setNoAliasToLoop(LVer.getVersionedLoop());
     Changed = true;
   }
-  // Delete allocated memory.
-  delete CurAST;
   return Changed;
 }
 
 char LoopVersioningLICM::ID = 0;
 INITIALIZE_PASS_BEGIN(LoopVersioningLICM, "loop-versioning-licm",
                       "Loop Versioning For LICM", false, false)
 INITIALIZE_PASS_DEPENDENCY(AAResultsWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(GlobalsAAWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(LCSSAWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(LoopAccessLegacyAnalysis)
 INITIALIZE_PASS_DEPENDENCY(LoopInfoWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(LoopSimplify)
 INITIALIZE_PASS_DEPENDENCY(ScalarEvolutionWrapperPass)
-INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
 INITIALIZE_PASS_END(LoopVersioningLICM, "loop-versioning-licm",
                     "Loop Versioning For LICM", false, false)
 
 Pass *llvm::createLoopVersioningLICMPass() { return new LoopVersioningLICM(); }