[PATCH] [Polly] Added support for modulo expressions

[Johannes Doerfert]

I added some comments and will change than patch accordingly. I will also split two smaller parts and remove some test cases for which I unfortunately can't remember the exact reason I added them. Anyway, thanks for the review.


================
Comment at: lib/Analysis/ScopInfo.cpp:207
@@ +206,3 @@
+  if (auto *OpTR = dyn_cast<SCEVTruncateExpr>(OpS))
+    return isl_pw_aff_mod_val(visit(OpTR->getOperand()),
+                              isl_val_int_from_si(Ctx, ModCst));
----------------
grosser wrote:
> The operand of this truncate expression and correspondingly the dividend of this remainder operation can become negative. It may be worth a test case (example attached) and a comment on why aff_mod_val models this pattern
> correctly despite the fact that for negative dividends of the srem expression, it is not the right choice. 
> 
> {F356464}
I'll add that.

================
Comment at: lib/Analysis/ScopInfo.cpp:326
@@ +325,3 @@
+           "Modulo expressions are only valid for a constant right hand side");
+    RHSVal = isl_aff_get_constant_val(RHSAff);
+
----------------
grosser wrote:
> This is a lot of code, just to get a constant. Assuming we run an instruction simplification pass before Polly, can it really happen that RHS is not an explicit constant? If we assume the RHS is always a constant, we could simplify this by directly converting BO->getOperand(1) to an isl_pw_aff.
> 
> Also, as previously discussed, the use of isl's aff_mod_val() is incorrect for cases where the LHS is negative, as the semantics of isl_aff_mod and srem differ.
The second concern is important and I will correct that. Regarding the first, I'm not sure, maybe if we check this in the ScopDetection we can just assume a constant RHS here. I'm not sure which is better and do not have any arguments for one or another, so we should just choose one. (Or we do a isa<ConstantInt>(BO->getOperand(1)) here?)

================
Comment at: lib/Support/SCEVValidator.cpp:98
@@ -97,2 +97,3 @@
+    return *this;
   }
 
----------------
grosser wrote:
> The change of the return type does not seem to be used in this patch.
> 
> Moving class -> const is an obvious improvement. As it is unrelated to the patch, I would suggest to commit it directly as "obvious".
This was an artifact (I used it at some point but apperently I changed it again). I will commit the const part and forget about the return type

================
Comment at: lib/Support/SCEVValidator.cpp:386
@@ +385,3 @@
+        if (!Op0.isValid())
+          return ValidatorResult(SCEVType::INVALID);
+
----------------
grosser wrote:
> It would be nice to add a corresponding DEBUG statement to this INVALID instruction, which explains why the statement is refused (this is done consistently in this class).
I can change this to return Op0 if to make it clear that the DEBUG message was already produced in the recursive step.
Or did I miss something?

================
Comment at: lib/Support/SCEVValidator.cpp:389
@@ +388,3 @@
+        ValidatorResult Op1 = visit(SE.getSCEV(I->getOperand(1)));
+        if (!Op1.isValid() || !Op1.isINT())
+          return ValidatorResult(SCEVType::INVALID);
----------------
grosser wrote:
> Is the "!Op1.isValid() ||" part here needed?
> 
> Also, the patch refuses this piece of code which was previously accepted:
> 
> ```
> void foo(float *A, long a, long b) {
> 	for (long i = 0; i < (a % b); i++)
> 		A[i] = 1;
> }
> ```
> 
Good points. I'll address both.

================
Comment at: test/DeadCodeElimination/non-affine-but-modulo-and-affine-mix.ll:15
@@ -14,3 +10,1 @@
-; CHECK: for (int c1 = 0; c1 <= 1023; c1 += 1)
-; CHECK:   Stmt_S1(c1);
 ; CHECK: for (int c1 = 0; c1 <= 1023; c1 += 1)
----------------
grosser wrote:
> Maybe make this a CHECK-NOT: Stmt_S1?
> 
Agreed.

================
Comment at: test/ScopInfo/NonAffine/non_affine_but_modulo_condition_8.ll:14
@@ +13,3 @@
+; CHECK:     Scattering :=
+; CHECK:         { Stmt_if_then[i0] -> scattering[0, i0, 0] };
+; CHECK:     ReadAccess := [Reduction Type: +]
----------------
grosser wrote:
> The test cases fail ATM for me, because of the additional '0's in the scattering
I'll change it ...

http://reviews.llvm.org/D5293

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/