[polly] r244606 - Revise the simplification of regions
Tobias Grosser via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 12 02:08:00 PDT 2015
On 08/12/2015 10:07 AM, Johannes Doerfert wrote:
> On 08/12, Tobias Grosser wrote:
>> On 08/12/2015 09:38 AM, Johannes Doerfert wrote:
>>> On 08/12, Michael Kruse via llvm-commits wrote:
>>>> Two corrections:
>>>> First, such PHI nodes can only happen in the entry node, since
>>>> buildPHIAccesses checks the origin of the incoming edges (not of the
>>>> operands which I assumed).
>>> The operands are not the relevant part of the PHI semantics but the
>>> control flow is. There are examples for this in the PHI codegen
>>> discussion(s) we had on the list.
>>>> Sich PHI nodes can only appear in the entry
>>>> node. So I think solutions 1 and 3 do work.
>>> What is wrong with adding a pre SCoP block for all entry block PHI
>>> nodes? That solved the problem before and I cannot see the negative side
>>> effects and it needs only a minimal amount of code.
>> Right, that would also be a correct solution. I now just committed a
>> slightly different fix in r244721, which seems equivalent simple.
>> Maybe you can have a look. We can change the fix if another solution
>> is clearly better.
> I see two benefits and one drawback for the pre entry PHI block:
> + Less accesses in the SCoP (compile time)
Right. My feeling is that as this cases is pretty rare, this may not
be that much of an issue.
> + Less dependences, hence restrictions, in the SCoP (runtime)
A read-only access to an array that is never written in the SCoP
should not trigger any additional dependences. The only exception is
if we have a basic block that just has this PHI node and then forwards
its value somewhere else. However, I also don't think this restricts
the set of transformations we can do.
> - Not 100% accurate if Polly as an analyzer would also be required to
> give information about scalars (not only memory).
This is actually what pushed me to this solution (besides minor consistency
considerations). For auto-parallelization and such, it is sometimes useful
to know which scalar values are read in the region. Obviously, we do still
not model all scalar values and could possibly also handle this case by
moving the PHIs out of the region before code generating, but this would
just be another place where we would need to think about it.
I am currently pretty happy with what we have now, as your PHI code generation
just transparently handles this.
>>>> This all doesn't look like designed deliberately. And it is what I
>>>> meant with make simplifyRegion "stable" in D11867.
>>> In the original PHI codegen patch there was a detection of PHI nodes
>>> that were at some point part of the region (in the entry block) and
>>> split out of it by one of the region simplification methods just before
>>> the codegen part. Once detected these PHI's were deliberately ignored in
>>> the block codegen as they were no longer part of the region (SCoP).
>>> I cannot argue for the simplification and splitting stuff but the PHI
>>> codegen was aware of such PHI nodes and was handling them at some point.
>> To my understanding there was no need to handle them in the PHI codegen
>> as they were always moved out of the scop before we actually did code
>> generation. This is not the case any more, so we now model these kind of
>> corner-case PHI nodes in r244721. I doubt the additional modeling has
>> any cost and it seems indeed even simpler than a special PHI-node
>> move function (which by itself would already be rather simple).
> There was a check (and a comment) that PHI nodes could have been moved
> out of the SCoP in which case we do not need to generate some code. I
> don't know if it's still there but it was at some point.
I searched git log -p of lib/Analysis and lib/Codegen for the term 'move'
and did not find anything related that has changed in 2015. Maybe I missed
>> Overall, I think this bug was rather easy to solve after Michael provided
>> some nice reduced test cases and a good analysis.
> Agreed, but the analysis did not contain the initial (and for me
> trivial) solution that would not have changed the behaviour of Polly
> with regards to entry block PHI nodes at all.
Right. Thanks for pointing this out. The right approach would have been to
make this restructuring not change the behavior of Polly and then propose
a separate patch that adds the PHI modeling when we need it. Sorry for having
Overall, do you think we can leave the code as it is or would you prefer to
re-instantiate the old behavior?
More information about the llvm-commits