<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Exchange Server">
<!-- converted from rtf -->
<style><!-- .EmailQuote { margin-left: 1pt; padding-left: 4pt; border-left: #800000 2px solid; } --></style>
</head>
<body>
<font face="Calibri" size="2"><span style="font-size:11pt;">
<div>Thanks Hal for looking into this again.</div>
<div> </div>
<div> </div>
<div style="padding-left:36pt;">I've started to look at this again; I apologize for the delay...</div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:125</div>
<div style="padding-left:36pt;">@@ +124,3 @@</div>
<div style="padding-left:36pt;">+</div>
<div style="padding-left:36pt;">+// Sets input string as meta data to loop latch terminator instruction.</div>
<div style="padding-left:36pt;">+static void setLoopMetaData(Loop *CurLoop, const char *MDString) {</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">I see that you've marked this as done; does this patch need to be updated to reflect the change?</div>
<div>This comments was old comment on moving ‘cloneLoop’ to utility.</div>
<div>But now it’s no more required as that function got removed, and using LoopVersioning utility for cloning etc.</div>
<div> </div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:223</div>
<div style="padding-left:36pt;">@@ +222,3 @@</div>
<div style="padding-left:36pt;">+</div>
<div style="padding-left:36pt;">+/// \brief Check loop structure and confirms it's good for LoopVersioningLICM.</div>
<div style="padding-left:36pt;">+bool LoopVersioningLICM::legalLoopStructure() {</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">Can you be more specific? Are these checks reflecting limitation of the versioning infrastructure? </div>
<div>Some part of checks are repeated, but I kept them to complete legality at one place.</div>
<div>Not sure it’s a right decision.</div>
<div> </div>
<div style="padding-left:36pt;">Heuristics for profitability? Both? Please specifically explain this in the comments for each check.</div>
<div>Sure will update.</div>
<div> </div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:291</div>
<div style="padding-left:36pt;">@@ +290,3 @@</div>
<div style="padding-left:36pt;">+ // 2) PointerSet shouldn't have pointers more then RuntimeMemoryCheckThreshold</div>
<div style="padding-left:36pt;">+ // 3) Make sure AliasSets doesn't have any must alias set.</div>
<div style="padding-left:36pt;">+ for (const auto &I : *CurAST) {</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">Why? Must-alias with what? Why does it matter if there happens to be something that aliases with something else, if there is a third thing that only may alias with the first two?</div>
<div>Case where 2 pointers are must-aliased, there runtime bound check always give same result. In such cases there is no need for runtime checks and loop versioning.</div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:308</div>
<div style="padding-left:36pt;">@@ +307,3 @@</div>
<div style="padding-left:36pt;">+ Value *Ptr = A.getValue();</div>
<div style="padding-left:36pt;">+ // alias tracker should have pointers of same data type.</div>
<div style="padding-left:36pt;">+ typeCheck = (typeCheck && (SomePtr->getType() == Ptr->getType()));</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">alias -> Alias</div>
<div>Will correct.</div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:333</div>
<div style="padding-left:36pt;">@@ +332,3 @@</div>
<div style="padding-left:36pt;">+ }</div>
<div style="padding-left:36pt;">+ // Atleast 2 pointers needed for runtime check.</div>
<div style="padding-left:36pt;">+ if (PointerSet.size() <= 1) {</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">Atleast -> At least</div>
<div>Will correct.</div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:356</div>
<div style="padding-left:36pt;">@@ +355,3 @@</div>
<div style="padding-left:36pt;">+ const bool IsAnnotatedParallel = CurLoop->isAnnotatedParallel();</div>
<div style="padding-left:36pt;">+ // We dont support call instructions. however, we ignore few intrinsic</div>
<div style="padding-left:36pt;">+ // and libfunc callsite. We don't allow non-intrinsic, non-libfunc callsite</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">Why not? I understand that the vectorizer has these checks, but I don't see why this belongs in an LICM pass?</div>
<div>There is a possibility that call may modify aliasing behavior, which may defeat the purpose of versioning & runtime checks.</div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">Repository:</div>
<div style="padding-left:36pt;"> rL LLVM</div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;"><a href="http://reviews.llvm.org/D9151"><font color="#0563C1"><u>http://reviews.llvm.org/D9151</u></font></a></div>
<div> </div>
<div> </div>
</span></font>
</body>
</html>