[PATCH] D34982: [Polly][WIP] Fully-Indexed static expansion

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/Support/RegisterPasses.cpp:149
+static cl::opt<bool> FullyIndexedStaticExpansion(
+    "polly-expand-mse",
+    cl::desc("Fully expand the memory accesses of the detected Scops"),
----------------
I'm ok with the switch name, but doesn't the e in "mse" already stand for "expansion" (therefore expand-mse is short for "expand-maximal-array-expansion")


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:307-308
+           "The upper bound is not a positive integer.");
+    assert(UpperBound.le(isl::val(CurrentAccessMap.get_ctx(),
+                                  std::numeric_limits<unsigned>::max())) &&
+           "The upper bound overflow a long.");
----------------
This assertion fails on Windows (and 32 bit platforms): The `isl::val` constructor takes a `long`, which is 32 bit this platforms. UINT_MAX exceeds its range.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:309
+                                  std::numeric_limits<unsigned>::max())) &&
+           "The upper bound overflow a long.");
+    Sizes.push_back(UpperBound.get_num_si() + 1);
----------------
It has been tested for the range of an `unsigned int`.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:310
+           "The upper bound overflow a long.");
+    Sizes.push_back(UpperBound.get_num_si() + 1);
+  }
----------------
If `UpperBound.get_num_si()` is `UINT_MAX`, you get an overflow when adding +1.


================
Comment at: test/MaximalStaticExpansion/read_from_original.ll:1
+; RUN: opt -polly-canonicalize %loadPolly -polly-mse -analyze < %s | FileCheck %s
+; RUN: opt -polly-canonicalize %loadPolly -polly-mse -pass-remarks-analysis="polly-mse" -analyze < %s 2>&1| FileCheck %s --check-prefix=MSE
----------------
Nice new testcase!


https://reviews.llvm.org/D34982