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

[Malhar via Phabricator via llvm-commits]

malharJ added inline comments.


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

Looking at the definition of `MemoryDepChecker::getDependences()`,
it can return a nullptr if `RecordDependences` is false.
and that happens when number of dependences exceeds `MaxDependences`. 
Given that `max-dependences` is a command line option, it could have any value.


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

We will still need the check to see if we can find a dependency type that is unsafe for vectorization.

Just the presence of `Deps` is not sufficient to say above will be satisfied, because it can contain 
Forward or Backwardvectorizable dependencies too (but we dont care about emitting any remark for them as they can be vectorized)


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2158
+    SourceLoc = I->getDebugLoc();
+    if (auto *DD = dyn_cast<Instruction>(isa<MemSetInst>(I)
+                                             ? cast<MemSetInst>(I)->getRawDest()
----------------
sdesmalen wrote:
> 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?
Ok, I've changed it to `dyn_cast_or_null` (I think you had already mentioned it an earlier review comment but I missed it).
Can we please avoid adding more tests, from what I understand it may not be very crucial to test getPointerOperand()
as we just avoid emitting debug location information if it returns null pointer.


Also, I tried writing a test case for memset case and I found that it doesnt even make sense to have a case here for memset.
Apparently LAA does not consider any instruction that is not a load/store 
I see this code in `LoopAccessInfo::analyzeLoop()`

```
        if (!St) {
          recordAnalysis("CantVectorizeInstruction", St)
              << "instruction cannot be vectorized";
          HasComplexMemInst = true;
          continue;
        }
```
So it looks like a different remark is emitted and an early exit is taken because memset is considered as a "complex" memory instruction.

considering the above, I have removed the memset part from the patch.


================
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
+
----------------
sdesmalen wrote:
> 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)
I'm not very clear on the issue here .. they are already printed on separate lines ?

Regardless I've updated the tests to print the entire message.
(The reason I was avoiding it earlier was because it was too long a message, and didn't appeal to me as part of the remark.
However, it was not added by my patch so I will not be changing it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108371