<div>15.01.2012, 14:16, "Chandler Carruth" <chandlerc@google.com>:</div><blockquote type="cite"><div>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 style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid #cccccc;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;</blockquote><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></blockquote><div>UnswitchedValsIt variables are not stored anywhere. And collection is not modified during iterator lifetime. I defined iterators for shorter code and for faster refactoring if</div><div>we'll need to replace type in future. This iterator is used when I cloning data from old collection to the new one (line #317):</div><div>UnswitchedValsMap& Insts = OldLoopProps.UnswitchedVals;</div><div>...</div><div>for (UnswitchedValsIt I = Insts.begin(); I != Insts.end(); ++I) {</div><div>...</div><div>š</div><blockquote type="cite"><div><div>š</div><blockquote style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid #cccccc;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.</blockquote><div>I don't understand why this is a performance consideration as opposed to an iteration order consideration?</div></div></blockquote><div>The first is that DenseMap works faster than std::map for small collections.</div><div>The second: we need access to the current loop properties several times during iteration. To avoid extra lookup job I keep pointer to the properties of current loop.</div><div>The problem why I can't use DenseMap here is next. During single loop processing iteration we may add new loops, so LoopsProperties collection is changed. In a case of DenseMap all items are invalidated (DenseMap::grow, DenseMap.h, line 389). </div><blockquote type="cite"><div><div>š</div><blockquote style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid #cccccc;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),</blockquote><div>Please use 0 rather than NULL in LLVM code.</div></div></blockquote><div>OK.</div><blockquote type="cite"><div><div>š</div><div>š</div><blockquote style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid #cccccc;padding-left:1ex;">+ š š š šMaxSize(Threshold)<br /> + š š š{}<br /> +<br /> + š š š// Analyze loop. Check its size, calculate is it possible to unswitchš</blockquote><blockquote style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid #cccccc;padding-left:1ex;">+ š š š// it. Returns true if we can unswitch this loop.</blockquote><div>These should all be doxygen comments.š</div></div></blockquote><div>Did you mean three slashes (///)? </div><blockquote type="cite"><div><div>š</div><blockquote style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid #cccccc;padding-left:1ex;">+ š š šbool countLoop(const Loop* L);</blockquote><div>Always attach the '*' or the '&' to the variable name in LLVM code. This is a problem throughout this patch.</div></div></blockquote><div>ok.</div><blockquote type="cite"><div><div>š</div><blockquote style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid #cccccc;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.</blockquote><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></blockquote><div>Hm... When I removed this lines, the patch becames ugly. Actually I added new class here and moved contents from existing classes. After removal of these lines the patch looks like I renamed old class to the new one and made some crazy reorderings. So I tried to reduce O(N) in this case for more comfortable patch reading.</div><div>š</div><div>If you're not against my comments to your remarks I can apply the fixes now.</div><div>š</div><div>Thanks!</div><div>š</div><div>-Stepan.</div>