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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Hi @malharJ thanks for splitting up and simplifying this patch, this is an improvement. I left a few more comments, and also found that the remark itself needs a bit of work to separate it from the original message. Maybe you can also update the tests to check the full context, to ensure this no longer happens?



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2141
+  auto Deps = getDepChecker().getDependences();
+  if (!Deps)
+    return;
----------------
Can this be an assert?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2148
+      });
+  if (Found == Deps->end())
+    return;
----------------
Can this be an assert?


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2154
+
+  // Emit remark for first unsafe dependence
+  DebugLoc SourceLoc;
----------------
nit: please move this code below the switch


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2158
+    SourceLoc = I->getDebugLoc();
+    if (auto *DD = dyn_cast<Instruction>(isa<MemSetInst>(I)
+                                             ? cast<MemSetInst>(I)->getRawDest()
----------------
This dyn_cast will cause a segfault if the value returned by `getPointerOperand` is a `nullptr`. This needs to be `dyn_cast_or_null`. It would be good to have a test for this case.

Also, there is no test for the MemSetInst either. Can you add one?


================
Comment at: llvm/test/Transforms/LoopVectorize/memory-dep-remarks.ll:120
+; CHECK: remark: source.c:48:14: loop not vectorized: unsafe dependent memory operations in loop.
+; CHECK-NEXT: Backward loop carried data dependence. Memory location is the same as accessed at source.c:47:5
+
----------------
When I try this out with Clang, I see:
```Use #pragma loop distribute(enable) to allow loop distribution to attempt to isolate the offending operations into a separate loopBackward loop carried data dependence```

The issue here is the `loopBackward` (no period and space between remarks)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108371