<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 review. </div>
<div> </div>
<div>I’ll incorporate your comments and come back soon.</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 style="padding-left:36pt;">></div>
<div style="padding-left:36pt;">> 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 style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">Alright, I have a better understanding of what you're doing now. Your technique for generating the versioned loop is to check that all pointer access in the loop are independent (and, thus, don't alias), and guarded by that check,
you reach the versioned variant of the loop where the aliasing metadata asserts that all access are mutually independent. The follow-up LICM pass then actually does the hoisting.</div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">I agree this makes sense, because if you had multiple aliasing domains then you'd still not necessarily be able to hoist the potentially-loop-invariant accesses out of the loop. Please, however, explain all of this near the CurAST
iteration code so that it is clear why you have this restriction.</div>
<div> </div>
<div>Sure I’ll mention about this.</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;"> </div>
<div style="padding-left:36pt;">> @@ +355,3 @@</div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">> + const bool IsAnnotatedParallel = CurLoop->isAnnotatedParallel();</div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">> + // We dont support call instructions. however, we ignore few intrinsic</div>
<div style="padding-left:36pt;"> </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;">> ----------------</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 style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">> </div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">> 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;">Ah, good point. However, please then replace this check with an appropriate AA getModRef-type check for whether the call might alias with the relevant pointers (if the function, for example, does not alias with any of the loop-invariant
accesses, then it can't affect what you're trying to do, although you'll need to be somewhat more careful about adding the noalias metadata to those functions, etc.).</div>
<div> </div>
<div>I’m not sure that itself would be sufficient, consider indirect calls probably need to consider function pointer in runtime check.</div>
<div>Also there is a possibility that in call argument one of the runtime pointer escaped, in such cases it become more difficult to ensure correctness.</div>
<div>That’s why for simplicity I considered vectorizer approach to consider only few functions.</div>
<div> </div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:97</div>
<div style="padding-left:36pt;">@@ +96,3 @@</div>
<div style="padding-left:36pt;">+#define DEBUG_TYPE "loop-versioning-licm"</div>
<div style="padding-left:36pt;">+#define ORIGINAL_LOOP_METADATA "LoopVersioningLICMOriginalLoop"</div>
<div style="padding-left:36pt;">+#define VERSION_LOOP_METADATA "LoopVersioningLICMVersionLoop"</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">please use metadata names more consistent with our general naming scheme for standard metadata (llvm.loop.whatever). You should need only one metadata type, to disable licm-based versioning, it should be documented in the LangRef,
and you can add it to both original and versioned loops once this pass has run.</div>
<div> </div>
<div>Sure I’ll use only one meta data.</div>
<div>Will update LangRef as well.</div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:426</div>
<div style="padding-left:36pt;">@@ +425,3 @@</div>
<div style="padding-left:36pt;">+ }</div>
<div style="padding-left:36pt;">+ // Loop should have at least invariant store instruction.</div>
<div style="padding-left:36pt;">+ if (!InvariantCounter) {</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">Should comment say "load or store instruction"?</div>
<div> </div>
<div>Ah, this comment needs to be updated.</div>
<div> </div>
<div> </div>
<div>Regards,</div>
<div>Ashutosh</div>
<div> </div>
<div> </div>
</span></font>
</body>
</html>