[PATCH] D38722: Added Remarks for Loop Versioning LICM Pass

[Adam Nemet via Phabricator via llvm-commits]

anemet added a subscriber: vivekvpandya.
anemet added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:410
+                                 CurLoop->getStartLoc(), CurLoop->getHeader())
+        << "Runtime checks are more than threshold !!\n");
     return false;
----------------
Deepak_Porwal wrote:
> anemet wrote:
> > It's usually useful to output the actual numbers.  See how we do that in Inliner.cpp where we include the cost and the threshold.
> > 
> > Also please use the new closure API with emit.  Again, see the inliner or https://reviews.llvm.org/D38285.
> Working on necessary changes. 
> 
> Out of curiosity would like to know the reason for using new Closure API with emit for some places and not for all places (as in Inliner.cpp). 
> Out of curiosity would like to know the reason for using new Closure API with emit for some places and not for all places (as in Inliner.cpp).

I only converted a few in order to test the approach.  @vivekvpandya is converting most other places.

We should convert all unless the emit call is already guarded by allowExtraAnalysis.  There is also one more subtle case that cannot be converted when instead of the passname PrintAll is passed.


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:412
+                                 CurLoop->getStartLoc(), CurLoop->getHeader())
+        << "Runtime checks " << NV("RuntimeChecks",
+                                  LAI->getNumRuntimePointerChecks())
----------------
Number of runtime checks (<number>) exceeds threshold (<threshold>) 


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:486-493
+  if (!legalLoopInstructions(ORE)) {
     DEBUG(dbgs()
           << "    Loop instructions not suitable for LoopVersioningLICM\n\n");
+    ORE->emit([&]() {
+        return OptimizationRemarkMissed(DEBUG_TYPE, "IllegalLoopInst",
+                                 CurLoop->getStartLoc(), CurLoop->getHeader())
+        << "Illegal Loop Instruction\n";
----------------
Can this actually return the instruction as well and then we can use that for the location rather than the entire loop.


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:497-504
   if (!legalLoopMemoryAccesses()) {
     DEBUG(dbgs()
           << "    Loop memory access not suitable for LoopVersioningLICM\n\n");
+    ORE->emit([&]() {
+        return OptimizationRemarkMissed(DEBUG_TYPE, "IllegalLoopMemoryAccess",
+                                 CurLoop->getStartLoc(), CurLoop->getHeader())
+        << "Illegal Loop memory access\n";
----------------
Same here if possible.


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:512
+                               CurLoop->getStartLoc(), CurLoop->getHeader())
+      << "Loop Versioning LICM can be applied\n";
+  });
----------------
Versioned loop for LICM.  Please remove the new line everywhere.


Repository:
  rL LLVM

https://reviews.llvm.org/D38722