[Polly] Fix parallelism check for reductions

[Tobias Grosser]

On 26/06/2014 02:24, Doerfert, Johannes wrote:
> Hey Tobi,
>
> This is a patch we need to discuss in detail.
> I basically tried to fix the loop reversal and modulo problems we discussed earlier.
>
> Do you see any problems whit this way of check for broken dependences?

I have some comments, but feel free to ignore what you believe is
unnecessary.

>       // Reduction dependences
> -    TYPE_RED = 0x8,
> +    TYPE_RED = 1 << 3,
>
> -    // All (validity) dependences
> -    TYPE_ALL = (TYPE_WAR | TYPE_RAW | TYPE_WAW)

I would have committed this change separately.

> +    // Transitive closure of the reduction dependences
> +    TYPE_TC_RED = 1 << 4,
> +
> +    // Reversed transitive closure of the reduction dependences
> +    TYPE_RTC_RED = 1 << 5,

Why do you expose these dependences at all?  I personally would have
stored the union of those two in TYPE_RED and would have removed the
negative dependences from TYPE_RED any time getDependences is called
(using the schedule to determin what negative means).

> +/// @brief Fix all dimension of @p Zero to 0 and add it to @p user
> +static int fixSetToZero(__isl_take isl_set *Zero, void *user) {
> +  isl_union_set **User = (isl_union_set **)user;
> +  for (unsigned i = 0; i < isl_set_dim(Zero, isl_dim_set); i++)
> +    Zero = isl_set_fix_si(Zero, isl_dim_set, i, 0);
> +  *User = isl_union_set_add_set(*User, Zero);
> +  return 0;
> +}
> +
>   /// @brief Compute the privatization dependences for a given dependency @p Map
>   ///
>   /// Privatization dependences are widened original dependences which originate
> @@ -132,14 +141,27 @@ void Dependences::collectInfo(Scop &S, isl_union_map **Read,
>   ///       S1[i0] -> S2[] : i0 >= 0 and i0 <= 1023}
>   ///   RED:
>   ///     { S1[i0] -> S1[1 + i0] : i0 >= 0 and i0 <= 1022 }
> +///
> +/// Note: This function also computes the (reverse) transitive closure of the
> +///       reduction dependences.
>   void Dependences::addPrivatizationDependences() {
> -  isl_union_map *PrivRAW, *PrivWAW, *PrivWAR, *TransClosure;
> +  isl_union_map *PrivRAW, *PrivWAW, *PrivWAR;
>
>     // The transitive closure might be over approximated but we only use it to
>     // compute the privatization dependences. Thus, overapproximation will lead
> -  // "only" to more conservative privatization dependences.
> -  // FIXME: Take precautions to ensure only forward dependences are created.
> -  TransClosure = isl_union_map_transitive_closure(isl_union_map_copy(RED), 0);
> +  // "only" to more conservative privatization dependences. However, we take
> +  // precautions to ensure only forward dependences are created.
> +  TC_RED = isl_union_map_transitive_closure(isl_union_map_copy(RED), 0);
> +
> +  isl_union_set *UDeltas = isl_union_map_deltas(isl_union_map_copy(TC_RED));
> +  isl_union_set *Universe = isl_union_set_universe(isl_union_set_copy(UDeltas));
> +  isl_union_set *Zero = isl_union_set_empty(isl_union_set_get_space(Universe));
> +  isl_union_set_foreach_set(Universe, fixSetToZero, &Zero);

As the schedule space is a isl_set, but not a union_set you may be able
to avoid this helper function.

> +  isl_union_map *NonPositive = isl_union_set_lex_le_union_set(UDeltas, Zero);

You do not involve the schedule here? Only after applying the schedule
you can know if dependences are negative in time.


If you computed TC_RED as the union of TC_RED and RTC_RED before calling
this function, you an just use TC_RED here.

> @@ -371,7 +393,7 @@ bool Dependences::isParallelDimension(__isl_take isl_set *ScheduleSubset,
>
>     // FIXME: We can remove ignore reduction dependences in case we privatize the
>     //        memory locations the reduction statements reduce into.
> -  Deps = getDependences(TYPE_ALL | TYPE_RED);
> +  Deps = getDependences(TYPE_RAW | TYPE_WAW | TYPE_WAR | TYPE_RED);

Why do you use TYPE_RED here and not the TC_RED dependences?

(I know having two parallelism checks is a bad idea)

> -  isl_union_map *RedDeps = D->getDependences(Dependences::TYPE_RED);
> +  isl_union_map *RedDeps =
> +      D->getDependences(Dependences::TYPE_TC_RED | Dependences::TYPE_RTC_RED);

Does this work if we reverse the dependences?

  "schedule" : "[n] -> { Stmt_S0[i0] -> scattering[0, -i0, 0]: i0 % 2 = 
0; Stmt_S0[i0] -> scattering[2, -i0, 0]: i0 % 2 = 1 }"

Cheers,
Tobias