[PATCH] [Polly] Added support for modulo expressions

[Tobias Grosser]

I looked through the test files and I have the feeling there is a some redundancy. For example, the following files are identical, except for the constants used:

non_affine_but_modulo_access_1023.ll
non_affine_but_modulo_access_3.ll
non_affine_but_modulo_access_4.ll
non_affine_but_modulo_access_5.ll
non_affine_but_modulo_access_6.ll

non_affine_but_modulo_loop_bound_constant_7.ll
non_affine_but_modulo_loop_bound_constant_9.ll
non_affine_but_modulo_loop_bound_parameter.ll

non_affine_but_modulo_condition_3.ll
non_affine_but_modulo_condition_5.ll

non_affine_but_modulo_condition_4.ll
non_affine_but_modulo_condition_8.ll

Test coverage in general is good, but my feeling is that these tests may not increase test coverage, but rather distract from the test that cover the relevant cases.


================
Comment at: lib/Analysis/ScopInfo.cpp:76
@@ +75,3 @@
+  return 0;
+}
+
----------------
This would not be needed, if we just focus on constants in the RHS. (see below)

================
Comment at: lib/Analysis/ScopInfo.cpp:192
@@ +191,3 @@
+    const SCEV *OpARStart = OpAR->getStart();
+    const SCEV *OpARStep = OpAR->getStepRecurrence(SE);
+
----------------
This can be folded into the if below.

================
Comment at: lib/Analysis/ScopInfo.cpp:201
@@ +200,3 @@
+    const SCEV *NewAR = SE.getAddRecExpr(OpARStart, OpARStep, OpAR->getLoop(),
+                                         OpAR->getNoWrapFlags());
+    return isl_pw_aff_mod_val(visit(NewAR), isl_val_int_from_si(Ctx, ModCst));
----------------
This can be folded into the if above.

================
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));
----------------
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}

================
Comment at: lib/Analysis/ScopInfo.cpp:310
@@ +309,3 @@
+  Value *Unknown = Expr->getValue();
+  if (auto *BO = dyn_cast<BinaryOperator>(Unknown)) {
+    isl_pw_aff *RHS, *LHS;
----------------
I like the idea of extending our analysis across unknowns. This is indeed a very nice approach.

================
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);
+
----------------
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.

================
Comment at: lib/Support/SCEVValidator.cpp:98
@@ -97,2 +97,3 @@
+    return *this;
   }
 
----------------
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".

================
Comment at: lib/Support/SCEVValidator.cpp:386
@@ +385,3 @@
+        if (!Op0.isValid())
+          return ValidatorResult(SCEVType::INVALID);
+
----------------
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).

================
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);
----------------
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;
}
```


================
Comment at: test/DeadCodeElimination/non-affine-affine-mix.ll:9
@@ -8,3 +8,3 @@
 ;    }
 
 ; We unfortunately do need to execute all iterations of S1, as we do not know
----------------
Great that you preserve this file.

================
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)
----------------
Maybe make this a CHECK-NOT: Stmt_S1?


================
Comment at: test/ScopDetect/non_affine_but_modulo_access.ll:7
@@ -9,3 +6,3 @@
 ;      for (int i = 0; i < 1024; i++)
-;        A[i % 2] += i;
+;        A[i % 2] = A[i % 2 + 1];
 ;    }
----------------
Nice that we can detect this now!

================
Comment at: test/ScopInfo/NonAffine/indirect_array_write.ll:8
@@ -6,3 +7,3 @@
 ;     A[INDEX[i]] = i;
 ; }
 
----------------
Is this in any way related to modulos? If not and this just makes the filename more understandable, this could be committed separately.

================
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: +]
----------------
The test cases fail ATM for me, because of the additional '0's in the scattering

================
Comment at: test/ScopInfo/NonAffine/non_affine_but_modulo_loop_bound_constant_7.ll:16
@@ +15,3 @@
+;    }
+;
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
Interesting. I like this test case.

================
Comment at: test/ScopInfo/reduction_alternating_base.ll:12
@@ -11,3 +11,3 @@
 ;    }
 ;
 target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-n32-S64"
----------------
Nice!

http://reviews.llvm.org/D5293

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