[PATCH] D21124: Cache aware Loop Cost Analysis

[JF Bastien via llvm-commits]

jfb added a comment.

Quick review, nothing technical.


================
Comment at: include/llvm/Analysis/LoopCostAnalysis.h:43
@@ +42,3 @@
+
+// This data is mainly used for cache resue (spatial/temporal) aware
+// calculations.
----------------
Typo "resue"

================
Comment at: include/llvm/Analysis/LoopCostAnalysis.h:46
@@ +45,3 @@
+// TODO: This should probably go to a separate file with separate
+// implemenation if the size grows.
+class CacheData {
----------------
Typo "implemenation"

================
Comment at: include/llvm/Analysis/LoopCostAnalysis.h:50
@@ +49,3 @@
+  unsigned CacheSize;  // The size of cache.
+  enum CacheWay {
+    DIRECT,
----------------
`enum class` is nicer IMO. You can also rename it to Way instead of CacheWay.

================
Comment at: include/llvm/Analysis/LoopCostAnalysis.h:67
@@ +66,3 @@
+    Associativity = _Assoc;
+  }
+
----------------
This ctor isn't used. It looks like you could get rid of the default ctor and initCacheData if you have access to the target at construction time (do you? I'm not sure, it would be good to wire that up).

If you keep this ctor, it should be written as:
```
CacheData(unsigned LineSize, unsigned CacheSize,
          CacheData::CacheWay Associativity)
    : LineSize(LineSize), CacheSize(CacheSize), Associativity(Associativity) {}
```
You can't use identifiers starting with `_[A-Z]`, and you can reuse the same name.

================
Comment at: include/llvm/Analysis/LoopCostAnalysis.h:77
@@ +76,3 @@
+  void setAssociativity(CacheData::CacheWay Assoc)  { Associativity = Assoc; }
+  CacheData::CacheWay getAssociativity()  { return Associativity; }
+
----------------
The three setters above aren't used, and don't seem useful given that you have initCacheData.

================
Comment at: include/llvm/Analysis/LoopCostAnalysis.h:82
@@ +81,3 @@
+    // Set default values for generic data layout.
+    LineSize = 4;   // Statically setting for now.
+  }
----------------
Could you address this TODO? That's the part I'm most interested in :)
That could be a separate patch if you want to keep things minimal.

================
Comment at: include/llvm/Analysis/LoopCostAnalysis.h:91
@@ +90,3 @@
+// loop nest. 
+typedef SmallVector<Loop *, 2> LoopNest_t;
+
----------------
LLVM doesn't usually name things with "_t".

================
Comment at: include/llvm/Analysis/LoopCostAnalysis.h:97
@@ +96,3 @@
+public:
+  enum Order {
+    COLUMNMAJOR = 0,
----------------
enum class here as well.

================
Comment at: include/llvm/Analysis/LoopCostAnalysis.h:100
@@ +99,3 @@
+    ROWMAJOR
+  } AccessOrder;
+
----------------
Column major is never used in the code. Is it going to be added later?

================
Comment at: include/llvm/Analysis/LoopCostAnalysis.h:116
@@ +115,3 @@
+  /// The loops and their calculated loop costs.
+  std::map <Loop *, double> LoopCosts;
+
----------------
Weird format here. Could you run new code through `clang-format`?

================
Comment at: include/llvm/Analysis/LoopCostAnalysis.h:116
@@ +115,3 @@
+  /// The loops and their calculated loop costs.
+  std::map <Loop *, double> LoopCosts;
+
----------------
jfb wrote:
> Weird format here. Could you run new code through `clang-format`?
Wouldn't `float` be sufficient for this purpose?

================
Comment at: lib/Analysis/LoopCostAnalysis.cpp:22
@@ +21,3 @@
+
+/* - Build/test perfect loop nests - should probably be moved to Loop Utils - */
+// TODO: Rotated loops forming perfect nests cannot be relied on and hence not
----------------
?

================
Comment at: lib/Analysis/LoopCostAnalysis.cpp:35
@@ +34,3 @@
+  Loop *Subloop = *L->begin();
+  for (auto bb = L->block_begin(), bbe = L->block_end(); bb != bbe; ++bb) {
+    if (*bb == L->getHeader() || *bb == L->getLoopLatch())
----------------
Use a range-based for loop here. There are other places below where you should also use them.

================
Comment at: lib/Analysis/LoopCostAnalysis.cpp:161
@@ +160,3 @@
+void LoopCost::setTripCounts(LoopNest_t LN) {
+#define STATIC_TRIP_COUNT 1000
+
----------------
Use a `constexpr` for this, or a `cl::opt` if you want to be able to set it from the command line.

================
Comment at: lib/Analysis/LoopCostAnalysis.cpp:165
@@ +164,3 @@
+    unsigned TripCount = SCEV->getSmallConstantTripCount(L, L->getExitingBlock());
+    LoopTripCounts.push_back(std::make_pair(L, (TripCount)));
+  }
----------------
You can `push_back({L, TripCount})`

================
Comment at: lib/Analysis/LoopCostAnalysis.cpp:193
@@ +192,3 @@
+// This routine creates different Reference (GEP) Groups based on GEP's access
+// w.r.t. CacheLine. A GEP access is added to an existing group if it's access
+// lies in the same cache line; otherwise this GEP will form a new RefGroup.
----------------
"its access"

================
Comment at: lib/Analysis/LoopCostAnalysis.cpp:203
@@ +202,3 @@
+void LoopCost::CreateReferenceGroups(BasicBlock *bb) {
+  assert(bb);
+  for (auto &i : *bb) {
----------------
Delete?

================
Comment at: lib/Analysis/LoopCostAnalysis.cpp:204
@@ +203,3 @@
+  assert(bb);
+  for (auto &i : *bb) {
+    if (auto *GEP = dyn_cast<GetElementPtrInst>(&i)) {
----------------
Naming isn't consistent with other parts for the LLVM code (`I` and `BB`), which you seem to try to follow in general (e.g. `GEP`).

================
Comment at: test/Analysis/CostModel/X86/matmul-perfect-loopCost.ll:23
@@ +22,3 @@
+; CHECK-NEXT: Loop: for.cond1 Costs: 6.256252e+10
+; CHECK-NEXT: Loop: for.cond  Costs: 2.501750e+11
+; CHECK:      Loop: for.cond  TripCount: 5001
----------------
Is your algorithm stable enough that every platform will produce exactly the same `double` result? You may want to print out the double differently so it's easier to ignore bits of lower significance.


http://reviews.llvm.org/D21124