[PATCH] D108371: [LAA] Add Memory dependence remarks.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/LoopAccessAnalysis.h:19
 #include "llvm/Analysis/LoopAnalysisManager.h"
+#include "llvm/Analysis/OptimizationRemarkEmitter.h"
 #include "llvm/Analysis/ScalarEvolutionExpressions.h"
----------------
This change is unnecessary, keep the include in the .cpp


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2136
+    auto Deps = getDepChecker().getDependences();
+    if (!Deps)
+      return;
----------------
It seems like it would make things easier to read if the logic would be in a separate function, with proper documentation what it is supposed to do?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2138
+      return;
+    auto found = std::find_if(
+        Deps->begin(), Deps->end(), [](const MemoryDepChecker::Dependence &D) {
----------------
nit: variable names here start with upper cases


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2149
+
+    // Emit detailed remarks for each unsafe dependence
+    DebugLoc SourceLoc;
----------------
Is this true? Unless I miss something, this emits a remark for the first unsafe dependence?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2170
+    case MemoryDepChecker::Dependence::BackwardVectorizable:
+      llvm_unreachable("Unexpected dependency");
+    case MemoryDepChecker::Dependence::Backward:
----------------
nit: dependence instead of dependency, to be in line with the terminology used elsewhere in the file?


================
Comment at: llvm/test/Transforms/LoopVectorize/memory-dep-remarks.ll:1
+; RUN: opt -passes='loop(require<access-info>),function(loop-vectorize)' -disable-output -pass-remarks-analysis=loop-vectorize < %s 2>&1 | FileCheck %s
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
----------------
For remarks, it might be good to check the full yaml generated, as in `llvm/test/Transforms/LoopVectorize/X86/vectorization-remarks-missed.ll`


================
Comment at: llvm/test/Transforms/LoopVectorize/memory-dep-remarks.ll:16
+entry:
+  %cmp12 = icmp sgt i64 %n, 1
+  br i1 %cmp12, label %for.body, label %for.cond.cleanup
----------------
nit: check here not needed?


================
Comment at: llvm/test/Transforms/LoopVectorize/memory-dep-remarks.ll:19
+
+for.cond.cleanup:                                 ; preds = %for.body, %entry
+  ret void
----------------
nit: easier to read if this is the last block


================
Comment at: llvm/test/Transforms/LoopVectorize/memory-dep-remarks.ll:121
+entry:
+  %cmp.not19 = icmp slt i32 %n, 4
+  br i1 %cmp.not19, label %for.cond.cleanup, label %for.body.preheader
----------------
Is this needed? Same for other tests.


================
Comment at: llvm/test/Transforms/LoopVectorize/memory-dep-remarks.ll:126
+  %sub = add nsw i32 %n, -3
+  %0 = zext i32 %sub to i64
+  br label %for.body
----------------
is this needed?


================
Comment at: llvm/test/Transforms/LoopVectorize/memory-dep-remarks.ll:264
+!1 = !DIFile(filename: "source.c", directory: "")
+!2 = !{}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
----------------
can the debug lock be trimmed down a bit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108371