<div class="gmail_quote">On Sun, Jan 15, 2012 at 1:44 AM, Stepan Dyatkovskiy <span dir="ltr"><<a href="mailto:stpworld@narod.ru">stpworld@narod.ru</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: dyatkovskiy<br>
Date: Sun Jan 15 03:44:07 2012<br>
New Revision: 148215<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=148215&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=148215&view=rev</a><br>
Log:<br>
Fixup for r148132. Type replacement for LoopsProperties: from DenseMap to std::map, since we need to keep a valid pointer to properties of current loop.<br>
<br>
Message for r148132:<br>
LoopUnswitch: All helper data that is collected during loop-unswitch iterations was moved to separated class (LUAnalysisCache).<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Scalar/LoopUnswitch.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scalar/LoopUnswitch.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoopUnswitch.cpp?rev=148215&r1=148214&r2=148215&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoopUnswitch.cpp?rev=148215&r1=148214&r2=148215&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Scalar/LoopUnswitch.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scalar/LoopUnswitch.cpp Sun Jan 15 03:44:07 2012<br>
@@ -48,6 +48,7 @@<br>
#include "llvm/Support/Debug.h"<br>
#include "llvm/Support/raw_ostream.h"<br>
#include <algorithm><br>
+#include <map><br>
#include <set><br>
using namespace llvm;<br>
<br>
@@ -65,6 +66,61 @@<br>
cl::init(100), cl::Hidden);<br>
<br>
namespace {<br>
+<br>
+ class LUAnalysisCache {<br>
+<br>
+ typedef DenseMap<const SwitchInst*, SmallPtrSet<const Value *, 8> ><br>
+ UnswitchedValsMap;<br>
+<br>
+ typedef UnswitchedValsMap::iterator UnswitchedValsIt;<br></blockquote><div><br></div><div>Is this dense map safe for some reason? I'm deeply suspicious of *any* dense map that is being iterated over and which is keyed on pointers.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ struct LoopProperties {<br>
+ unsigned CanBeUnswitchedCount;<br>
+ unsigned SizeEstimation;<br>
+ UnswitchedValsMap UnswitchedVals;<br>
+ };<br>
+<br>
+ // Here we use std::map instead of DenseMap, since we need to keep valid<br>
+ // LoopProperties pointer for current loop for better performance.<br></blockquote><div><br></div><div>I don't understand why this is a performance consideration as opposed to an iteration order consideration?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ typedef std::map<const Loop*, LoopProperties> LoopPropsMap;<br>
+ typedef LoopPropsMap::iterator LoopPropsMapIt;<br>
+<br>
+ LoopPropsMap LoopsProperties;<br>
+ UnswitchedValsMap* CurLoopInstructions;<br>
+ LoopProperties* CurrentLoopProperties;<br>
+<br>
+ // Max size of code we can produce on remained iterations.<br>
+ unsigned MaxSize;<br>
+<br>
+ public:<br>
+<br>
+ LUAnalysisCache() :<br>
+ CurLoopInstructions(NULL), CurrentLoopProperties(NULL),<br></blockquote><div><br></div><div>Please use 0 rather than NULL in LLVM code.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ MaxSize(Threshold)<br>
+ {}<br>
+<br>
+ // Analyze loop. Check its size, calculate is it possible to unswitch </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+ // it. Returns true if we can unswitch this loop.<br>
</blockquote><div><br></div><div>These should all be doxygen comments. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ bool countLoop(const Loop* L);<br></blockquote><div><br></div><div>Always attach the '*' or the '&' to the variable name in LLVM code. This is a problem throughout this patch.</div><div><br></div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ // Clean all data related to given loop.<br>
+ void forgetLoop(const Loop* L);<br>
+<br>
+ // Mark case value as unswitched.<br>
+ // Since SI instruction can be partly unswitched, in order to avoid<br>
+ // extra unswitching in cloned loops keep track all unswitched values.<br>
+ void setUnswitched(const SwitchInst* SI, const Value* V);<br>
+<br>
+ // Check was this case value unswitched before or not.<br>
+ bool isUnswitched(const SwitchInst* SI, const Value* V);<br>
+<br>
+ // Clone all loop-unswitch related loop properties.<br>
+ // Redistribute unswitching quotas.<br>
+ // Note, that new loop data is stored inside the VMap.<br>
+ void cloneData(const Loop* NewLoop, const Loop* OldLoop,<br>
+ const ValueToValueMapTy& VMap);<br>
+ };<br>
+<br>
class LoopUnswitch : public LoopPass {<br>
LoopInfo *LI; // Loop information<br>
LPPassManager *LPM;<br>
@@ -72,21 +128,21 @@<br>
// LoopProcessWorklist - Used to check if second loop needs processing<br>
// after RewriteLoopBodyWithConditionConstant rewrites first loop.<br>
std::vector<Loop*> LoopProcessWorklist;<br>
-<br>
+<br>
+ // TODO: This few lines are here for cosmetic purposes only.<br>
+ // Will be removed with the next commit.<br></blockquote><div><br></div><div>In the future, please just delete the lines before re-submitting your patch. Adding these kinds of todo comments to reduce local merges shifts the pain from you (O(1)) to everyone reading the commit list (O(N)).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
struct LoopProperties {<br>
unsigned CanBeUnswitchedCount;<br>
unsigned SizeEstimation;<br>
};<br>
<br>
+ // TODO: This few lines are here for cosmetic purposes only.<br>
+ // Will be removed with the next commit.<br>
typedef DenseMap<const Loop*, LoopProperties> LoopPropsMap;<br>
typedef LoopPropsMap::iterator LoopPropsMapIt;<br>
- LoopPropsMap LoopsProperties;<br>
+ LoopPropsMap LoopsProperties;<br>
<br>
- // Max size of code we can produce on remained iterations.<br>
- unsigned MaxSize;<br>
-<br>
- // FIXME: Consider custom class for this.<br>
- std::map<const SwitchInst*, SmallPtrSet<const Value *,8> > UnswitchedVals;<br>
+ LUAnalysisCache BranchesInfo;<br>
<br>
bool OptimizeForSize;<br>
bool redoLoop;<br>
@@ -106,7 +162,7 @@<br>
public:<br>
static char ID; // Pass ID, replacement for typeid<br>
explicit LoopUnswitch(bool Os = false) :<br>
- LoopPass(ID), MaxSize(Threshold), OptimizeForSize(Os), redoLoop(false),<br>
+ LoopPass(ID), OptimizeForSize(Os), redoLoop(false),<br>
currentLoop(NULL), DT(NULL), loopHeader(NULL),<br>
loopPreheader(NULL) {<br>
initializeLoopUnswitchPass(*PassRegistry::getPassRegistry());<br>
@@ -132,24 +188,7 @@<br>
private:<br>
<br>
virtual void releaseMemory() {<br>
-<br>
- LoopPropsMapIt LIt = LoopsProperties.find(currentLoop);<br>
-<br>
- if (LIt != LoopsProperties.end()) {<br>
- LoopProperties& Props = LIt->second;<br>
- MaxSize += Props.CanBeUnswitchedCount * Props.SizeEstimation;<br>
- LoopsProperties.erase(LIt);<br>
- }<br>
-<br>
- // We need to forget about all switches in the current loop.<br>
- // FIXME: Do it better than enumerating all blocks of code<br>
- // and see if it is a switch instruction.<br>
- for (Loop::block_iterator I = currentLoop->block_begin(),<br>
- E = currentLoop->block_end(); I != E; ++I) {<br>
- SwitchInst* SI = dyn_cast<SwitchInst>((*I)->getTerminator());<br>
- if (SI)<br>
- UnswitchedVals.erase(SI);<br>
- }<br>
+ BranchesInfo.forgetLoop(currentLoop);<br>
}<br>
<br>
/// RemoveLoopFromWorklist - If the specified loop is on the loop worklist,<br>
@@ -161,15 +200,6 @@<br>
LoopProcessWorklist.erase(I);<br>
}<br>
<br>
- /// For new loop switches we clone info about values that was<br>
- /// already unswitched and has redundant successors.<br>
- /// Note, that new loop data is stored inside the VMap.<br>
- void CloneUnswitchedVals(const ValueToValueMapTy& VMap,<br>
- const BasicBlock* SrcBB);<br>
-<br>
- bool CountLoop(const Loop* L);<br>
- void CloneLoopProperties(const Loop* NewLoop, const Loop* OldLoop);<br>
-<br>
void initLoopData() {<br>
loopHeader = currentLoop->getHeader();<br>
loopPreheader = currentLoop->getLoopPreheader();<br>
@@ -201,6 +231,112 @@<br>
<br>
};<br>
}<br>
+<br>
+// Analyze loop. Check its size, calculate is it possible to unswitch<br>
+// it. Returns true if we can unswitch this loop.<br>
+bool LUAnalysisCache::countLoop(const Loop* L) {<br>
+<br>
+ std::pair<LoopPropsMapIt, bool> InsertRes =<br>
+ LoopsProperties.insert(std::make_pair(L, LoopProperties()));<br>
+<br>
+ LoopProperties& Props = InsertRes.first->second;<br>
+<br>
+ if (InsertRes.second) {<br>
+ // New loop.<br>
+<br>
+ // Limit the number of instructions to avoid causing significant code<br>
+ // expansion, and the number of basic blocks, to avoid loops with<br>
+ // large numbers of branches which cause loop unswitching to go crazy.<br>
+ // This is a very ad-hoc heuristic.<br>
+<br>
+ // FIXME: This is overly conservative because it does not take into<br>
+ // consideration code simplification opportunities and code that can<br>
+ // be shared by the resultant unswitched loops.<br>
+ CodeMetrics Metrics;<br>
+ for (Loop::block_iterator I = L->block_begin(),<br>
+ E = L->block_end();<br>
+ I != E; ++I)<br>
+ Metrics.analyzeBasicBlock(*I);<br>
+<br>
+ Props.SizeEstimation = std::min(Metrics.NumInsts, Metrics.NumBlocks * 5);<br>
+ Props.CanBeUnswitchedCount = MaxSize / (Props.SizeEstimation);<br>
+ MaxSize -= Props.SizeEstimation * Props.CanBeUnswitchedCount;<br>
+ }<br>
+<br>
+ if (!Props.CanBeUnswitchedCount) {<br>
+ DEBUG(dbgs() << "NOT unswitching loop %"<br>
+ << L->getHeader()->getName() << ", cost too high: "<br>
+ << L->getBlocks().size() << "\n");<br>
+<br>
+ return false;<br>
+ }<br>
+<br>
+ // Be careful. This links are good only before new loop addition.<br>
+ CurrentLoopProperties = &Props;<br>
+ CurLoopInstructions = &Props.UnswitchedVals;<br>
+<br>
+ return true;<br>
+}<br>
+<br>
+// Clean all data related to given loop.<br>
+void LUAnalysisCache::forgetLoop(const Loop* L) {<br>
+<br>
+ LoopPropsMapIt LIt = LoopsProperties.find(L);<br>
+<br>
+ if (LIt != LoopsProperties.end()) {<br>
+ LoopProperties& Props = LIt->second;<br>
+ MaxSize += Props.CanBeUnswitchedCount * Props.SizeEstimation;<br>
+ LoopsProperties.erase(LIt);<br>
+ }<br>
+<br>
+ CurrentLoopProperties = NULL;<br>
+ CurLoopInstructions = NULL;<br>
+}<br>
+<br>
+// Mark case value as unswitched.<br>
+// Since SI instruction can be partly unswitched, in order to avoid<br>
+// extra unswitching in cloned loops keep track all unswitched values.<br>
+void LUAnalysisCache::setUnswitched(const SwitchInst* SI, const Value* V) {<br>
+ (*CurLoopInstructions)[SI].insert(V);<br>
+}<br>
+<br>
+// Check was this case value unswitched before or not.<br>
+bool LUAnalysisCache::isUnswitched(const SwitchInst* SI, const Value* V) {<br>
+ return (*CurLoopInstructions)[SI].count(V);<br>
+}<br>
+<br>
+// Clone all loop-unswitch related loop properties.<br>
+// Redistribute unswitching quotas.<br>
+// Note, that new loop data is stored inside the VMap.<br>
+void LUAnalysisCache::cloneData(const Loop* NewLoop, const Loop* OldLoop,<br>
+ const ValueToValueMapTy& VMap) {<br>
+<br>
+ LoopProperties& NewLoopProps = LoopsProperties[NewLoop];<br>
+ LoopProperties& OldLoopProps = *CurrentLoopProperties;<br>
+ UnswitchedValsMap& Insts = OldLoopProps.UnswitchedVals;<br>
+<br>
+ // Reallocate "can-be-unswitched quota"<br>
+<br>
+ --OldLoopProps.CanBeUnswitchedCount;<br>
+ unsigned Quota = OldLoopProps.CanBeUnswitchedCount;<br>
+ NewLoopProps.CanBeUnswitchedCount = Quota / 2;<br>
+ OldLoopProps.CanBeUnswitchedCount = Quota - Quota / 2;<br>
+<br>
+ NewLoopProps.SizeEstimation = OldLoopProps.SizeEstimation;<br>
+<br>
+ // Clone unswitched values info:<br>
+ // for new loop switches we clone info about values that was<br>
+ // already unswitched and has redundant successors.<br>
+ for (UnswitchedValsIt I = Insts.begin(); I != Insts.end(); ++I) {<br>
+ const SwitchInst* OldInst = I->first;<br>
+ Value* NewI = VMap.lookup(OldInst);<br>
+ const SwitchInst* NewInst = cast_or_null<SwitchInst>(NewI);<br>
+ assert(NewInst && "All instructions that are in SrcBB must be in VMap.");<br>
+<br>
+ NewLoopProps.UnswitchedVals[NewInst] = OldLoopProps.UnswitchedVals[OldInst];<br>
+ }<br>
+}<br>
+<br>
char LoopUnswitch::ID = 0;<br>
INITIALIZE_PASS_BEGIN(LoopUnswitch, "loop-unswitch", "Unswitch loops",<br>
false, false)<br>
@@ -286,7 +422,7 @@<br>
<br>
// Probably we reach the quota of branches for this loop. If so<br>
// stop unswitching.<br>
- if (!CountLoop(currentLoop))<br>
+ if (!BranchesInfo.countLoop(currentLoop))<br>
return false;<br>
<br>
// Loop over all of the basic blocks in the loop. If we find an interior<br>
@@ -324,7 +460,7 @@<br>
// some not yet unswitched. Let's find the first not yet unswitched one.<br>
for (unsigned i = 1; i < NumCases; ++i) {<br>
Constant* UnswitchValCandidate = SI->getCaseValue(i);<br>
- if (!UnswitchedVals[SI].count(UnswitchValCandidate)) {<br>
+ if (!BranchesInfo.isUnswitched(SI, UnswitchValCandidate)) {<br>
UnswitchVal = UnswitchValCandidate;<br>
break;<br>
}<br>
@@ -356,75 +492,6 @@<br>
return Changed;<br>
}<br>
<br>
-/// For new loop switches we clone info about values that was<br>
-/// already unswitched and has redundant successors.<br>
-/// Not that new loop data is stored inside the VMap.<br>
-void LoopUnswitch::CloneUnswitchedVals(const ValueToValueMapTy& VMap,<br>
- const BasicBlock* SrcBB) {<br>
-<br>
- const SwitchInst* SI = dyn_cast<SwitchInst>(SrcBB->getTerminator());<br>
- if (SI && UnswitchedVals.count(SI)) {<br>
- // Don't clone a totally simplified switch.<br>
- if (isa<Constant>(SI->getCondition()))<br>
- return;<br>
- Value* I = VMap.lookup(SI);<br>
- assert(I && "All instructions that are in SrcBB must be in VMap.");<br>
- UnswitchedVals[cast<SwitchInst>(I)] = UnswitchedVals[SI];<br>
- }<br>
-}<br>
-<br>
-bool LoopUnswitch::CountLoop(const Loop* L) {<br>
- std::pair<LoopPropsMapIt, bool> InsertRes =<br>
- LoopsProperties.insert(std::make_pair(L, LoopProperties()));<br>
-<br>
- LoopProperties& Props = InsertRes.first->second;<br>
-<br>
- if (InsertRes.second) {<br>
- // New loop.<br>
-<br>
- // Limit the number of instructions to avoid causing significant code<br>
- // expansion, and the number of basic blocks, to avoid loops with<br>
- // large numbers of branches which cause loop unswitching to go crazy.<br>
- // This is a very ad-hoc heuristic.<br>
-<br>
- // FIXME: This is overly conservative because it does not take into<br>
- // consideration code simplification opportunities and code that can<br>
- // be shared by the resultant unswitched loops.<br>
- CodeMetrics Metrics;<br>
- for (Loop::block_iterator I = L->block_begin(),<br>
- E = L->block_end();<br>
- I != E; ++I)<br>
- Metrics.analyzeBasicBlock(*I);<br>
-<br>
- Props.SizeEstimation = std::min(Metrics.NumInsts, Metrics.NumBlocks * 5);<br>
- Props.CanBeUnswitchedCount = MaxSize / (Props.SizeEstimation);<br>
- MaxSize -= Props.SizeEstimation * Props.CanBeUnswitchedCount;<br>
- }<br>
-<br>
- if (!Props.CanBeUnswitchedCount) {<br>
- DEBUG(dbgs() << "NOT unswitching loop %"<br>
- << L->getHeader()->getName() << ", cost too high: "<br>
- << L->getBlocks().size() << "\n");<br>
-<br>
- return false;<br>
- }<br>
- return true;<br>
-}<br>
-<br>
-void LoopUnswitch::CloneLoopProperties(<br>
- const Loop* NewLoop, const Loop* OldLoop) {<br>
-<br>
- LoopProperties& OldLoopProps = LoopsProperties[OldLoop];<br>
- LoopProperties& NewLoopProps = LoopsProperties[NewLoop];<br>
-<br>
- --OldLoopProps.CanBeUnswitchedCount;<br>
- unsigned Quota = OldLoopProps.CanBeUnswitchedCount;<br>
- NewLoopProps.CanBeUnswitchedCount = Quota / 2;<br>
- OldLoopProps.CanBeUnswitchedCount = Quota - Quota / 2;<br>
-<br>
- NewLoopProps.SizeEstimation = OldLoopProps.SizeEstimation;<br>
-}<br>
-<br>
/// isTrivialLoopExitBlock - Check to see if all paths from BB exit the<br>
/// loop with no side effects (including infinite loops).<br>
///<br>
@@ -529,7 +596,7 @@<br>
<br>
// Check that it was not unswitched before, since already unswitched<br>
// trivial vals are looks trivial too.<br>
- if (UnswitchedVals[SI].count(CaseVal))<br>
+ if (BranchesInfo.isUnswitched(SI, CaseVal))<br>
continue;<br>
LoopExitBB = LoopExitCandidate;<br>
if (Val) *Val = CaseVal;<br>
@@ -743,11 +810,6 @@<br>
for (unsigned i = 0, e = LoopBlocks.size(); i != e; ++i) {<br>
BasicBlock *NewBB = CloneBasicBlock(LoopBlocks[i], VMap, ".us", F);<br>
<br>
- // Inherit simplified switches info for NewBB<br>
- // We needn't pass NewBB since its instructions are already contained<br>
- // inside the VMap.<br>
- CloneUnswitchedVals(VMap, LoopBlocks[i]);<br>
-<br>
NewBlocks.push_back(NewBB);<br>
VMap[LoopBlocks[i]] = NewBB; // Keep the BB mapping.<br>
LPM->cloneBasicBlockSimpleAnalysis(LoopBlocks[i], NewBB, L);<br>
@@ -760,7 +822,11 @@<br>
<br>
// Now we create the new Loop object for the versioned loop.<br>
Loop *NewLoop = CloneLoop(L, L->getParentLoop(), VMap, LI, LPM);<br>
- CloneLoopProperties(NewLoop, L);<br>
+<br>
+ // Recalculate unswitching quota, inherit simplified switches info for NewBB,<br>
+ // Probably clone more loop-unswitch related loop properties.<br>
+ BranchesInfo.cloneData(NewLoop, L, VMap);<br>
+<br>
Loop *ParentLoop = L->getParentLoop();<br>
if (ParentLoop) {<br>
// Make sure to add the cloned preheader and exit blocks to the parent loop<br>
@@ -1075,7 +1141,7 @@<br>
BasicBlock *SISucc = SI->getSuccessor(DeadCase);<br>
BasicBlock *Latch = L->getLoopLatch();<br>
<br>
- UnswitchedVals[SI].insert(Val);<br>
+ BranchesInfo.setUnswitched(SI, Val);<br>
<br>
if (!SI->findCaseDest(SISucc)) continue; // Edge is critical.<br>
// If the DeadCase successor dominates the loop latch, then the<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br>