[PATCH] D108371: [LAA] Add Memory dependence remarks.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 28 09:53:37 PST 2021
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
More information about the llvm-commits
mailing list