<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 Philip for this review.</div>
<div> </div>
<div style="padding-left:36pt;">Drive by review. A few comments here or there.</div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">Meta comments:</div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">- Code structure and naming is somewhat confusing. Attention to factoring of functions and method names would help.</div>
<div style="padding-left:36pt;">- This feels like too large a patch to easily review. I would strongly prefer that you split the patch into the smallest possible piece (i.e. it works on trivial cases only), then extend. Given Hal has already started reviewing
this, I'll defer to his preferences here, but I'm unlikely to signoff on a patch this large and complex.</div>
<div> </div>
<div>I’ll try to divide this patch at least at logical level.</div>
<div>Also as suggested there are possibly few functions that can be moved as utility. </div>
<div>I’ll come-up with a separate patch for them.</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;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:3</div>
<div style="padding-left:36pt;">@@ +2,3 @@</div>
<div style="padding-left:36pt;">+//</div>
<div style="padding-left:36pt;">+// The LLVM Compiler Infrastructure</div>
<div style="padding-left:36pt;">+//</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">Please follow the standard header format. See LICM as an example.</div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">It would help to have a trivial example here. The shortest possible while loop which shows the transform would make the discussion much more clear.</div>
<div style="padding-left:36pt;"> </div>
<div>Sure will update it and add an example.</div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:57</div>
<div style="padding-left:36pt;">@@ +56,3 @@</div>
<div style="padding-left:36pt;">+#include "llvm/ADT/StringExtras.h"</div>
<div style="padding-left:36pt;">+#include "llvm/Analysis/ScalarEvolutionExpander.h"</div>
<div style="padding-left:36pt;">+#include "llvm/Transforms/Utils/BasicBlockUtils.h"</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">Include order should be sorted.</div>
<div> </div>
<div>Sure will sort the included headers and update.</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:78</div>
<div style="padding-left:36pt;">@@ +77,3 @@</div>
<div style="padding-left:36pt;">+</div>
<div style="padding-left:36pt;">+/// Loop Versioning invariant threshold</div>
<div style="padding-left:36pt;">+static cl::opt<unsigned></div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">What is an "invariant threshold"? You probably need a better name for this.</div>
<div> </div>
<div><font face="Verdana" size="2"><span style="font-size:10pt;">This option is to control the profitability of loop versioning for LICM.</span></font></div>
<div><font face="Verdana" size="2"><span style="font-size:10pt;">It checks invariant load & store vs total instruction of loop. If invariants </span></font></div>
<div><font face="Verdana" size="2"><span style="font-size:10pt;">are more than defined threshold then only go for LICM loop versioning. </span></font></div>
<div><font face="Verdana" size="2"><span style="font-size:10pt;">Threshold is defined in percentage with a default value 25%.</span></font></div>
<div> </div>
<div>How about “-licm-versioning-threshold” or “-licm-versioning-invariant-threshold” ?</div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:124</div>
<div style="padding-left:36pt;">@@ +123,3 @@</div>
<div style="padding-left:36pt;">+/// \brief Recursively clone the specified loop and all of its children,</div>
<div style="padding-left:36pt;">+/// mapping the blocks with the specified map.</div>
<div style="padding-left:36pt;">+static Loop *cloneLoop(Loop *L, Loop *PL, ValueToValueMapTy &VM, LoopInfo *LI,</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">I believe this is duplicated code with LICM. Please fine a place to put a utility function instead. Transforms/Utils/Loops.h would be fine.</div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">Ideally, this would be split into it's own patch.</div>
<div> </div>
<div>Sure I’ll move this to a utility.</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:145</div>
<div style="padding-left:36pt;">@@ +144,3 @@</div>
<div style="padding-left:36pt;">+/// return the induction operand of the gep pointer.</div>
<div style="padding-left:36pt;">+static Value *stripGetElementPtr(Value *Ptr, ScalarEvolution *SE, Loop *Lp) {</div>
<div style="padding-left:36pt;">+ GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(Ptr);</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">A better name would help here.</div>
<div style="padding-left:36pt;">Don't we have similar code in CGP?</div>
<div style="padding-left:36pt;"> </div>
<div>Similar function exists in LoopVectorizer will change it as a utility and use it.</div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:161</div>
<div style="padding-left:36pt;">@@ +160,3 @@</div>
<div style="padding-left:36pt;">+</div>
<div style="padding-left:36pt;">+/// \brief Look for a cast use of the passed value.</div>
<div style="padding-left:36pt;">+static Value *getUniqueCastUse(Value *Ptr, Loop *Lp, Type *Ty) {</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">Er, what does this actually do? The Ty and L params make me thing this isn't about finding a unique use...</div>
<div> </div>
<div>Similar function exists in LoopVectorizer will change it as a utility and use it.</div>
<div> </div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:686</div>
<div style="padding-left:36pt;">@@ +685,3 @@</div>
<div style="padding-left:36pt;">+/// +----------+</div>
<div style="padding-left:36pt;">+Loop *LoopVersioningLICM::versionizeLoop(LPPassManager &LPM) {</div>
<div style="padding-left:36pt;">+ std::vector<BasicBlock *> LoopBlocks;</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">I suspect there is code which can and should be commoned with unswitch here.</div>
<div> </div>
<div>I’ll check and come back to you.</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:826</div>
<div style="padding-left:36pt;">@@ +825,3 @@</div>
<div style="padding-left:36pt;">+ // Only interested in load & stores</div>
<div style="padding-left:36pt;">+ if (!it->mayReadFromMemory() && !it->mayWriteToMemory())</div>
<div style="padding-left:36pt;">+ continue;</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">Your comment and code disagree. Are you intentionally handling RMW operations?</div>
<div> </div>
<div>Will update comments here, we are handling RMW operations.</div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:833</div>
<div style="padding-left:36pt;">@@ +832,3 @@</div>
<div style="padding-left:36pt;">+ NoAliases.push_back(NewScope);</div>
<div style="padding-left:36pt;">+ // Set no-alias for current instruction.</div>
<div style="padding-left:36pt;">+ I->setMetadata(</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">The loop structure is very odd here. It looks like later instructions end up with more noalias markers than earlier ones? I suspect that's not what you want. </div>
<div> </div>
<div>Here we want to make no-alias to instructions, yes the last one will have more no-alias.</div>
<div>But its required to make instruction no-alias, i.e. loop has 3 instruction I1, I2, I3.</div>
<div>Then here we are setting no-alias property like below:</div>
<div>I1 No-Alias-1</div>
<div>I2 No-Alias-1, No-Alias-2</div>
<div>I2 No-Alias-1, No-Alias-2, No-Alias-3</div>
<div> </div>
<div>My understanding may be wrong here, if you have any better way of doing it then please suggest.</div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:871</div>
<div style="padding-left:36pt;">@@ +870,3 @@</div>
<div style="padding-left:36pt;">+ BasicBlock *BB = *I;</div>
<div style="padding-left:36pt;">+ if (LI->getLoopFor(BB) == L) // Ignore blocks in subloops.</div>
<div style="padding-left:36pt;">+ CurAST->add(*BB); // Incorporate the specified basic block</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">Why?</div>
<div> </div>
<div>As we are only interested in the inner most loop, it’s overhead creating AST for sub loops.</div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:889</div>
<div style="padding-left:36pt;">@@ +888,3 @@</div>
<div style="padding-left:36pt;">+ // Delete newly created loop from loop pass manager.</div>
<div style="padding-left:36pt;">+ LPM.deleteLoopFromQueue(VerLoop);</div>
<div style="padding-left:36pt;">+ Changed = true;</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">Why?</div>
<div> </div>
<div>This need to be removed as we already introduced metadata to ensure not revisiting same loop.</div>
<div> </div>
<div> </div>
<div style="padding-left:36pt;">================</div>
<div style="padding-left:36pt;">Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:893</div>
<div style="padding-left:36pt;">@@ +892,3 @@</div>
<div style="padding-left:36pt;">+ // Delete allocated memory.</div>
<div style="padding-left:36pt;">+ delete CurAST;</div>
<div style="padding-left:36pt;">+ delete PointerSet;</div>
<div style="padding-left:36pt;">----------------</div>
<div style="padding-left:36pt;">These can be stack allocated.</div>
<div style="padding-left:36pt;"> </div>
<div>Sure will make stack allocation.</div>
<div> </div>
<div style="padding-left:36pt;"><a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D9151&d=AwMFAg&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=7UNA5NSfMU44BYnfhTSh3KGPCi3-Pj6axbQsgD8pncg&s=Ye5-taz2BhAPGnUNPp7kSVjVXVUAlHs4a09l9oFIR0s&e="><font color="#0563C1"><u>http://reviews.llvm.org/D9151</u></font></a></div>
<div style="padding-left:36pt;"> </div>
<div style="padding-left:36pt;">EMAIL PREFERENCES</div>
<div style="padding-left:36pt;"> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFAg&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=7UNA5NSfMU44BYnfhTSh3KGPCi3-Pj6axbQsgD8pncg&s=O4yrrhSFXtEP_YXLEEA8v7X7x93gYHdC54ge1_D3Xvc&e="><font color="#0563C1"><u>http://reviews.llvm.org/settings/panel/emailpreferences/</u></font></a></div>
<div style="padding-left:36pt;"> </div>
<div> </div>
<div> </div>
<div> </div>
</span></font>
</body>
</html>