[PATCH] D24716: [Polly] DeLICM/DePRE (WIP)

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Michael,

this now looks a lot more focused. Very cool. I just had a quick look before work today and only came up with some minor comments so far. I will give this a deeper look this WE to prepare myself for Stockholm. While scanning I also saw that some comments might need updating. MK_* is still used and some grammar fixes. I will mark them in my next iteration.

Best,
Tobias



================
Comment at: include/polly/Support/GICHelper.h:21
 #include "isl/ctx.h"
+#include "isl/id.h"
 #include "isl/map.h"
----------------
Not needed.


================
Comment at: lib/Analysis/ScopInfo.cpp:123
     cl::desc("Count statements with scalar accesses as not optimizable"),
-    cl::Hidden, cl::init(true), cl::cat(PollyCategory));
+    cl::Hidden, cl::init(false), cl::cat(PollyCategory));
 
----------------
We should not change the default as of yet. (I know this was just a remainder of the full patch).


================
Comment at: lib/External/isl/imath/gmp_compat.c:691
   size_t num_words, num_missing_bytes;
-  ssize_t word_offset;
+  ptrdiff_t word_offset;
   unsigned char* dst;
----------------
These seem to be leftover changes which can be dropped.


================
Comment at: lib/External/isl/imath/gmp_compat.c:770
   size_t num_digits;
-  ssize_t word_offset;
+  ptrdiff_t word_offset;
   const unsigned char *src;
----------------
These seem to be leftover changes which can be dropped.


================
Comment at: lib/Support/GICHelper.cpp:180-182
+  replace(str, "+", ")");
+  replace(str, "%", "");
+  replace(str, "@", "");
----------------
grosser wrote:
> grosser wrote:
> > Meinersbur wrote:
> > > Adding the type to ISL names causes `%` and `@` to be in the middle of the string. `+` appears in `llvm::Value`s of floating-point constants.
> > As types don't seem to be needed, this change can be dropped from this patch, right?
> nothing (phabricator does not allow me to delete this comment)
This seems to be only needed for "Known" (The plus comes from FP values, AFAIK).


================
Comment at: lib/Support/RegisterPasses.cpp:166
                  cl::desc("Eliminate scalar loop carried dependences"),
-                 cl::Hidden, cl::init(false), cl::cat(PollyCategory));
+                 cl::Hidden, cl::init(true), cl::cat(PollyCategory));
 
----------------
I suggest to not enable this by default as of yet.


================
Comment at: test/DeLICM/reduction_preheader.ll:171
+; CHECK-NEXT:     Written : { [MemRef_A[i0] -> [i1{{\]\]}} : 0 <= i0 <= 1 and 15i0 <= i1 <= 13 + 15i0 and ((3*floor((i1)/3) = i1 and i1 <= 12 + 15i0) or (3*floor((-1 + i1)/3) = -1 + i1 and i1 > 15i0) or (3*floor((-2 + i1)/3) = -2 + i1 and 2 + 15i0 <= i1 <= 11 + 15i0)); [MemRef_A[i0] -> [14 + 15i0{{\]\]}} : 0 <= i0 <= 1 }
+; CHECK-NEXT: }
+; CHECK:      After accesses {
----------------
I suggest to not include the output above in the test. I am afraid this will change quite often -- such that we get a lot of noise. As long as the final result is identical to what we want, this seems sufficient.


================
Comment at: test/ScopInfo/licm_load.ll:1
-; RUN: opt %loadPolly -basicaa -loop-rotate -indvars       -polly-prepare \
-; RUN: -polly-invariant-load-hoisting=true -polly-scops -analyze < %s \
----------------
I would suggest to not delete the existing test cases in this change. Let's have a separate discussion if / why they are not needed any more and either drop them or move them to test/DeLICM, if appropriate.


https://reviews.llvm.org/D24716