[PATCH] D79564: [Loop-Vectorize] Emit more context in remarks for optimization recordThis extends the reportVectorizationFailure helper function with more information emitted after setExtraArgs. This avoids cluttering the compiler frontend remarks with low...

[Florian Hahn via Phabricator via llvm-commits]

fhahn added subscribers: thegameg, anemet, fhahn.
fhahn added reviewers: anemet, thegameg, fhahn.
fhahn added a comment.

Thanks for working on improving this!

Could you add some tests to check the additional context?

One thing we have to be a bit careful about is to not provide too much information tied to LLVM IR, as this can be confusing to users when viewing the remarks in the original source (which has no references to the IR). IIUC you are using allowExtraAnalysis as a proxy for whether the IR info should be included. Not sure what the current practice is in other passes, @anemet or @thegameg might have some input there.

An alternative might be to use the debug info to reference the original location in the source language. But there are also scenarios where this might fall apart (e..g if the problematic exits have been added by other passes).



================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorize.h:179
+    OptimizationRemarkEmitter *ORE, Loop *TheLoop, Instruction *I = nullptr,
+    std::function<void(OptimizationRemarkAnalysis &)> ExtraInfo = nullptr);
 
----------------
could this be llvm::function_ref?

also, might be good to rename ExtraInfo to something like GetExtraInfo, to have the name indicate that this is a function.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1087
+        for (unsigned i = 0; i < T->getNumSuccessors(); i++) {
+          if (T->getSuccessor(i) == Head) {
+            if (T->getNumSuccessors() > 1) {
----------------
things are quite deeply nested here. It might be better to flip the logic in the checks and have

```
if (T->getSuccessor(i) != Head)
  continue;
```

Same for T->getNumSuccessors.

Also if you are only interested in iteration over the successor blocks, you could use `successors(Latch)`.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1089
+            if (T->getNumSuccessors() > 1) {
+              std::string Description = "backedge (condition ";
+              Analysis << ore::NV(Description + (i ? "false)" : "true)"), T);
----------------
nit: StringREf?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1091
+              Analysis << ore::NV(Description + (i ? "false)" : "true)"), T);
+            } else {
+              Analysis << ore::NV("backedge", T);
----------------
nit: unnecessary {}


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1147
+    // included
+    if (Lp->getExitingBlock())
+      Term = Lp->getExitingBlock()->getTerminator();
----------------
Because of the check at line 1137, shouldn't we only reach here if both the latch and exiting block are != nullptr, i.e. wouldn't the exiting block always be used here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79564/new/

https://reviews.llvm.org/D79564