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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:218
+          emitRemark(SAI->getName() +
+                         " expansion leads to a partial read access.",
+                     MA->getAccessInstruction());
----------------
niosega wrote:
> Meinersbur wrote:
> > The consequence would not be a partial read access, but it would need to read the original value the element had before entering the SCoP. That's a special case similar to having more than one write.
> Are you sure ?
> 
> Because if I remember well. Let say that we are analyzing this code : 
> 
> ```
>   for (i = 0; i < Ni; i++) {
>     B[j] = i;
>     for (int j = 0; j<Nj; j++) {
>       B[j] = j;
>     }
>     A[i] = B[i]; 
> ```
> 
>  When I try to set the new access relation for the B read, the setNewAccessRelation method of class MemoryAccess failed with the assert "Partial READ accesses not supported". 
How were you able to do that? setNewAccessRelation accepts only an isl_map, not isl_union_map.

Let me explain in more detail.
```
for (int k = 0; k < M; k+=1) {
  for (int i = 0; i<= N/2; i+=1) {
S:  B[i] = i;
  }
  for (int i = 0; i<N; i+=1) {
T:  = ... B[i]
  }
}
```
The flow dependence would look something like:
```
{ T[k, i] -> S[k, i] : 0 < i <= N/2 }
```

We could naively expand B to B_expanded:
```
for (int k = 0; k < M; k+=1) {
  for (int i = 0; i<= N/2; i+=1) {
S:  B_expanded[k][i] = i;
  }
  for (int i = 0; i<N; i+=1) {
T:  = ... B_expanded[k][i]
  }
}
```
The problem here is that `B_expanded[k][i]` for i > N/2 never gets written (And T would read uninitialized memory). The flow dependence doesn't tell which instance of S wrote it in the first place!! If you try to apply it naively anyway using setNewAccessRelation, we need a source of the value for all instances of S, but we don't have one for `i > N/2`! This is way partial read accesses are unsupported.

The correct thing to do would be to read the value from the original array B (which then becomes read-only).
```
{ T[k,i] -> B_expanded[k,i] : i < N/2; T[k,i] -> B[i] : i>=N/2 }
```
This again is an isl_union_map (NOT a partial access since it is defined for all instances of T), which we currently do not support support.

Please try to understand what the problem with partial read accesses is. Not the partial read accesses are the problem, but the reason why you would want to use one.


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:307
+           "The upper bound is not a positive integer.");
+    Sizes.push_back(UpperBound.get_num_si() + 1);
+  }
----------------
niosega wrote:
> Meinersbur wrote:
> > [Nit] The UpperBound could overflow a long. Add an assertion for that?
> How can I efficiently check that there is an overflow ? 
`UpperBound.le(INT_MAX)` (I think there is no implicit conversion from int to isl::val, but you gget the idea)


================
Comment at: lib/Transform/MaximalStaticExpansion.cpp:328-331
+    auto Constraints = isl::constraint::alloc_equality(ls);
+    Constraints = Constraints.set_coefficient_si(isl::dim::out, dim, 1);
+    Constraints = Constraints.set_coefficient_si(isl::dim::in, dim, -1);
+    NewAccessMap = NewAccessMap.add_constraint(Constraints);
----------------
niosega wrote:
> Meinersbur wrote:
> > [Style] This could be simpler using
> > ```
> > NewAccessMap = NewAccessMap->equate(isl::dim::in, dim. isl::dim::out, dim);
> > ```
> > or, even, better, use `basic_map::equal`.
> I'd like to use isl_basic_map_equal but I did not find the documentation of this method on the online isl doc. There is also no example of uses in Polly. Can you explain me how it works ?
```
isl_map_space(SpaceMap.copy(), SpaceMap.dim(isl::in))
```
should get you a basic_map of that space where the `n_equal = SpaceMap.dim(isl::in)` in- and out-
 dimensions are equal. Something like.
```
{ Stmt[i0, i1] -> MemRef[o0, o1] : i0 = o0 and i1 = o1 }
```
However, no documention could mean that the function was not intended to be public.


https://reviews.llvm.org/D34982