[PATCH] D114336: [Polly] Generalize the pattern matching to the case of tensor contractions.

[Roman via Phabricator via llvm-commits]

gareevroman marked 19 inline comments as done.
gareevroman added inline comments.


================
Comment at: polly/lib/Transform/MatmulOptimizer.cpp:1093
+  isl::map PossibleTensor = isl::manage(Universe.copy());
+  for (int i = 0; i < static_cast<int>(Dimensions.size()); i++) {
+    const int InPos = Dimensions[i];
----------------
Meinersbur wrote:
> or introduce `intFromIslSize`.
As far I understand, Dimensions.size() returns a value of type size_t instead of a value of the type isl_size. So, in the new version I used the unsigned type to avoid the cast.


================
Comment at: polly/lib/Transform/MatmulOptimizer.cpp:1206-1207
+      return;
+    if (intersect(IandJIndexSet, TCI.P).size())
+      return;
+    TCI.WriteToC = MemAccessPtr;
----------------
Meinersbur wrote:
> This computes whether two sets a disjoint, it should not be required to compute the intersection.
That check is redundant. Thanks.


================
Comment at: polly/lib/Transform/MatmulOptimizer.cpp:1223
+                   const std::set<int> &IandJIndexSet,
+                   SmallVector<int, 30> Dimensions, TCInfoTy &TCI) {
+  if (!TCI.A) {
----------------
Meinersbur wrote:
> 
As far as I understand, we cannot do this here because of the assignment to TCI.ADimensions and TCI.BDimensions


================
Comment at: polly/lib/Transform/MatmulOptimizer.cpp:1261
+  TCI.ReadFromC = nullptr;
+  SmallVector<MemoryAccess *, 32> Accesses = getAccessesInOrder(*Stmt);
+  for (auto *MemA = Accesses.begin(); *MemA != TCI.WriteToC; MemA++) {
----------------
Meinersbur wrote:
> `getAccessesInOrder` requires `Stmt` to not be a RegionStmt. Please add a test for it.
I’ve added a check to containsOnlyTCAcc. Could you clarify how the test case should look like? Should it be a region statement that contains a matrix multiplication with right order of memory accesses?


================
Comment at: polly/lib/Transform/MatmulOptimizer.cpp:1342-1344
+  if ((unsignedFromIslSize(DepDelta.n_basic_set()) != 1) ||
+      !DepDelta.plain_get_val_if_fixed(isl::dim::set, Pos).is_one())
+    return false;
----------------
Meinersbur wrote:
> The check should not depend on `n_basic_set`, which is fragile and depends on whether on eg. `coalesce` was successful. Consider using something like `polly::getConstant`.
I think that this check was redundant. I’ve removed it.


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

https://reviews.llvm.org/D114336