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

[Evgeny Astigeevich via Phabricator via llvm-commits]

eastig added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:210-212
+  bool isLegalForVersioning(OptimizationRemarkEmitter *ORE);
   bool legalLoopStructure();
+  bool legalLoopInstructions(OptimizationRemarkEmitter *ORE);
----------------
What about to make ORE a class member?


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:397-399
+          return OptimizationRemarkMissed(DEBUG_TYPE, "IllegalLoopInst",
+                                 &Inst)
+          << " Illegal Loop Instruction";
----------------
Maybe it's better to use 'Unsafe' instead of 'Illegal'?


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:438-452
     DEBUG(dbgs()
           << "    Invariant load & store are less then defined threshold\n");
     DEBUG(dbgs() << "    Invariant loads & stores: "
                  << ((InvariantCounter * 100) / LoadAndStoreCounter) << "%\n");
     DEBUG(dbgs() << "    Invariant loads & store threshold: "
                  << InvariantThreshold << "%\n");
+    ORE->emit([&]() {
----------------
The message reported here is quite tricky. What I understand is: per cent of invariant memory operations in less then threshold per cent.
NV does not support FP values. Maybe it's better to add it instead of truncating 'static_cast<int>(InvariantThreshold)'. Diagnostics in this way do not provide actual information.


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:496-500
+    ORE->emit([&]() {
+      return OptimizationRemarkMissed(DEBUG_TYPE, "IllegalLoopInst",
+                               CurLoop->getStartLoc(), CurLoop->getHeader())
+          << " Loop instructions not suitable for LoopVersioningLICM";
+        });
----------------
Do we need this remark here because remarks related to legality of instructions have already been emitted?


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:507-511
+    ORE->emit([&]() {
+        return OptimizationRemarkMissed(DEBUG_TYPE, "IllegalLoopMemoryAccess",
+                                 CurLoop->getStartLoc(), CurLoop->getHeader())
+        << " Illegal Loop memory access";
+    });
----------------
Maybe legalLoopMemoryAccesses should emit remarks?


Repository:
  rL LLVM

https://reviews.llvm.org/D38722