[PATCH] D63459: Loop Cache Analysis

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:256
+
+  return dyn_cast<SCEVConstant>(RefCost)->getValue()->getSExtValue();
+}
----------------
RefCost is guaranteed to be a SCEVConstant here, so it would be better to use cast<> instead. Or even better 

```
if (auto ConstantCost = dyn_cast<SCEVCOnstant>(RefCost))
  return return ConstantCost->getValue()->getSExtValue();

LLVM_DEBUG(dbgs().indent(4)
             << "RefCost is not a constant! Setting to RefCost=InvalidCost "
                "(invalid value).\n");
return CacheCost::InvalidCost;
```


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:267
+
+  auto isOneDimensionalArray = [&](const SCEV *AccessFn, const Loop *L) {
+    const SCEVAddRecExpr *AR = dyn_cast<SCEVAddRecExpr>(AccessFn);
----------------
It looks like this function does not capture much and might be better as a separate member function?


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:274
+
+    // check that start and increment are not add recurrences.
+    const SCEV *Start = AR->getStart();
----------------
nit: Capitalize start of sentence.


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:280
+
+    // check that start and increment are both invariant in the loop.
+    if (!SE.isLoopInvariant(Start, L) || !SE.isLoopInvariant(Step, L))
----------------
nit: Capitalize start of sentence.


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:456
+  if (Root.getParentLoop()) {
+    LLVM_DEBUG(dbgs() << "Expecting the outermost loop in a loop nest");
+    return nullptr;
----------------
\n at the end?


================
Comment at: llvm/lib/Analysis/LoopCacheAnalysis.cpp:471
+  Function *F = Root.getHeader()->getParent();
+  DependenceInfo DI(F, &AR.AA, &AR.SE, &AR.LI);
+
----------------
This should be passed in I think and the users should request it via the pass manager.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63459