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

[Adam Nemet via Phabricator via llvm-commits]

anemet added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:512
+                               CurLoop->getStartLoc(), CurLoop->getHeader())
+      << "Loop Versioning LICM can be applied\n";
+  });
----------------
anemet wrote:
> Versioned loop for LICM.  Please remove the new line everywhere.
Can we mention how many checks we had to insert?


================
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([&]() {
----------------
Deepak_Porwal wrote:
> eastig wrote:
> > 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.
> I am not sure how to support FP value in NV
That should be trivial.  Take a look at how integers are emitted.


================
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";
+        });
----------------
Deepak_Porwal wrote:
> eastig wrote:
> > Do we need this remark here because remarks related to legality of instructions have already been emitted?
> We can keep this as it is informing about current loop location for the instruction.
We shouldn't emit multiple remarks for a single problem, if that is what you mean.  If legalLoopInstructions has already emitted something, please don't emit anything here.

If you want to mention the loop in the remark in legalLoopInstructions, you should be able to do that.  We can already insert DebugLoc into the stream, there should be examples in other passes.

Alternatively, you can use the loop location for the main DebugLoc of the remark and insert the Instruction via an NV.  Opt-viewer would display that as a link to the instruction in the code.


Repository:
  rL LLVM

https://reviews.llvm.org/D38722