[Polly][Refactor] IslAst [6/7]

[Johannes Doerfert]

Comments inlined.

>>The semantics for an outermost parallel loop have changed at two places.
>> I give details inline.
Thank you for catching that.

>> It is fine to suggest semantic changes, but we definitely do not want to
mix them with a refactoring change. 
This is true, and I didn't see/wanted that.

>> Also, if you would like to make semantic changes, we need a motivation
why the current approach is inappropriate and the new semantics are superior
in some way.
I usually try to do that, but it was not intended here. However I still
think we should get away from the IsInnermostParallel and
IsOutermostParallel flags.

> 0001-Refactor-Use-isParallel-and-isInnermost.patch
>
>
>  From 1c17cc6ce53ab9cbc865c46f21165f0548313fb3 Mon Sep 17 00:00:00 
> 2001
> From: Johannes Doerfert<jdoerfert at codeaurora.org>
> Date: Wed, 16 Jul 2014 16:35:55 -0700
> Subject: [PATCH] [Refactor] Use isParallel and isInnermost
>
>    + Change isInnermostParallel and isOuterParallel to combinations of
isParallel
>      and isInnermost.
>    + Perform the parallelism check on the innermost loop only once.
>    + Inline the markOpenmpParallel function.
>    + Rename all IslAstUserPayload * into Payload (was Info, NodeInfo and
Payload
>      before).

>> I think in general a one or two line description of why you change
something helps to understand why you believe something is beneficial.
What change are you hinting at? Most of them should be self-explanatory.

>> The printing changed semantics. Before this change, we never nested
pragma omp parallel for statements. Now we do. (See committed test case).
This is true, and I didn't see/wanted that. My question now is what kind of
behavior do we want?
Why do we have two annotations (llvm knows only parallel doesn't it?)
I would like to see every parallel loop annotated with "#pragma parallel"
and get rid of "#pragma simd".

>> > -  markOpenmpParallel(Build, BuildInfo, NodeInfo);
>> > +  // Test for parallelism only if we are not already inside a 
>> > + parallel loop
>> Point at the end of the sentence.


>> This says exactly what you are doing, but I have a hard time to
understand it. I think the point you want to make is that certain loops have
not yet been tested for parallelism and that we want to test them in case
they are innermost.
>> What about something like the following:
>> // Innermost loops that are surrounded by parallel loops have not yet //
been tested for parallelism. Test them here to ensure we check all //
innermost loops for parallelism.
Sure.

>> >   bool IslAstInfo::isInnermostParallel(__isl_keep isl_ast_node *Node) {
>> >     IslAstUserPayload *Payload = getNodePayload(Node);
>> > -  return Payload && Payload->IsInnermostParallel &&
>> > +  return Payload && Payload->IsInnermost && Payload->IsParallel &&
>> >            !Payload->IsReductionParallel;
>> >   }
>> Can we rename this function to isInnermostAndParallel to avoid confusion
with a check for an innermost loop that is parallel.
I don't understand what the second function is supposed to do, sorry.
And I don't know why it is now confusing, especially since the old attribute
was called IsInnermostParallel.

>> >   bool IslAstInfo::isOuterParallel(__isl_keep isl_ast_node *Node) {
>> >     IslAstUserPayload *Payload = getNodePayload(Node);
>> > -  return Payload && Payload->IsOutermostParallel &&
>> > +  return Payload && !Payload->IsInnermost && Payload->IsParallel &&
>> >            !Payload->IsReductionParallel;
>> >   }
>> I believe the semantics of this function changed. In case we have a
single parallel loop, this function would have returned 'true' before and
now returns 'false'. (When running -analyze we would still print a #pragma
omp parallel for statement, but I would prefer for the printing to use the
same function which we can use in the code generator to find the loop that
we want to omp parallelize.)
If I understand you correctly, you want to "omp parallelize" a loop without
regards if it is inner or outermost, right? If so the method isParallel is
the right choice not isOuterParallel.

--

Johannes Doerfert
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation