[polly] r244606 - Revise the simplification of regions

Tue Aug 11 16:12:49 PDT 2015

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). Sich PHI nodes can only appear in the entry
node. So I think solutions 1 and 3 do work.

Second, I now think it could not happen before my patch because if we
are not in the case that polly.entering.block would not be create,
SplitBlock in executeScopConditionally would have split the entering
block s.t. the PHI blocks would be left in the entering block. To stop
SplitEdge behave that way, it one must have one additional edge to the
entering, either from the inside or outside of the region. If from
inside, the PHI node must have a case for that block, making
buildPHIAccesses it not ignore anymore. If from outside,
simplifyRegion would have made a new entering block with the PHI
nodes.

This all doesn't look like designed deliberately. And it is what I
meant with make simplifyRegion "stable" in D11867.

I committed a test case for this (entry_with_trivial_phi.ll)

Michael

2015-08-11 23:02 GMT+02:00 Michael Kruse <llvm at meinersbur.de>:
> When I execute the test-suite on my local machine, I get some
> different error messages, but also for the same programs:
>
> oggenc:
> UNREACHABLE executed at
> /home/meinersbur/src/llvm-svn/tools/polly/lib/CodeGen/BlockGenerators.cpp:146!
>
> pifft:
> TIMING OUT PROCESS: Output/pifft.simple
>
> So I looked into the oggenc case first. The function that is failing
> is Laguerre_With_Deflation with the second detected scop:
>
> ; Function Attrs: nounwind uwtable
> define internal i32 @Laguerre_With_Deflation(float* %a, i32 %ord,
> float* %r) #0 {
> <snip>
>
> cleanup:                                          ; preds =
> %if.end.52, %for.end.24
>   %new.1 = phi double [ %new.0, %for.end.24 ], [ %sub55, %if.end.52 ]
>   %cleanup.dest.slot.0 = phi i32 [ 1, %for.end.24 ], [ %., %if.end.52 ]
>   %retval.2 = phi i32 [ -1, %for.end.24 ], [ %retval.1, %if.end.52 ]
>   switch i32 %cleanup.dest.slot.0, label %cleanup.88.loopexit [
>     i32 0, label %while.body
>     i32 9, label %while.end
>   ]
>
> while.end:                                        ; preds = %cleanup
>   %retval.2.lcssa122 = phi i32 [ %retval.2, %cleanup ]
>   %new.1.lcssa120 = phi double [ %new.1, %cleanup ]
>   %conv70 = fptrunc double %new.1.lcssa120 to float
>   %10 = add nsw i64 %indvars.iv131, -1
>   %arrayidx73 = getelementptr inbounds float, float* %r, i64 %10
>   store float %conv70, float* %arrayidx73, align 4
>   %cmp75.110 = icmp sgt i64 %indvars.iv131, 0
>   br i1 %cmp75.110, label %for.body.77.lr.ph, label %for.end.87
>
> <snip>
> }
>
>
> while.end is the scop's entry block. TempScopInfo ignores the lcssa
> PHI nodes because they use no value from inside the region.
>
> The BlockGenerator does does generate code for PHI nodes
> (copyPHIInstruction is empty). And when %conv70 is copied, there is no
> copied value for %new.1.lcssa120 , resulting in this unreachable.
>
>
> Before my commit, simplify region would create a new block
>
> polly.entering.block:                             ; preds = %cleanup
>   %retval.2.lcssa122 = phi i32 [ %retval.2, %cleanup ]
>   %new.1.lcssa120 = phi double [ %new.1, %cleanup ]
>   br label %polly.split_new_and_old
>
> that takes all the PHI nodes. But the condition why this block has
> been created has nothing to do with the PHI nodes (it is that the
> cleanup block has multiple outgoing edges). So I think it is just
> chance this just blows happens now.
>
>
>
> I can think of the following solutions:
>
> 1) Add a function to call in CodeGeneration after region
> simplification that removes PHI nodes with edges only from outside of
> the region. Not sure whether this can always work because the PHIs are
> not necessarily in this entry block and can have multiple incoming
> edges, so its value depend on the incoming edge, which is not
> statically known.
>
> 2) Make TempScopInfo not skip such PHI nodes. It follows that later
> passes also must handle such PHI nodes specially.
>
> 3) Let copyPHIInstruction detect this situation and create a value for
> this. Same problem as for 1) applies.
>
>
> IMHO 2) is the only working solution, but looks complicated. Maybe
> someone else has other ideas?
>
>
> Michael
>
>
>
>
> 2015-08-11 17:32 GMT+02:00 Michael Kruse <llvm at meinersbur.de>:
>> Looking into it. I find an assertion fail in the log:
>>
>> clang: /home/grosser/buildslave/perf-x86_64-penryn-O3-polly-fast/llvm.src/lib/Analysis/ScalarEvolution.cpp:3317:
>> const llvm::SCEV* llvm::ScalarEvolution::getSCEV(llvm::Value*):
>> Assertion `isSCEVable(V->getType()) && "Value is not SCEVable!"'
>> failed.
>>
>> Michael
>>
>>
>> 2015-08-11 17:08 GMT+02:00 Tobias Grosser <tobias at grosser.es>:
>>> On 08/11/2015 04:39 PM, Michael Kruse via llvm-commits wrote:
>>>>
>>>> Author: meinersbur
>>>> Date: Tue Aug 11 09:39:21 2015
>>>> New Revision: 244606
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=244606&view=rev
>>>> Log:
>>>> Revise the simplification of regions
>>>>
>>>> The previous code had several problems:
>>>>
>>>> For newly created BasicBlocks it did not (always) call
>>>> RegionInfo::setRegionFor in order to update its analysis. At the moment
>>>> RegionInfo does not verify its BBMap, but will in the future. This is fixed
>>>> by determining the region new BBs belong to and set it accordingly. The new
>>>> executeScopConditionally() requires accurate getRegionFor information.
>>>>
>>>> Which block is created by SplitEdge depends on the incoming and outgoing
>>>> edges of the blocks it connects, which makes handling its output more
>>>> difficult than it needs to be. Especially for finding which block has been
>>>> created an to assign a region to it for the setRegionFor problem above. This
>>>> patch uses an implementation for splitEdge that always creates a block
>>>> between the predecessor and successor. simplifyRegion has also been
>>>> simplified by using SplitBlockPredecessors instead of SplitEdge. Isolating
>>>> the entries and exits have been refectored into individual functions.
>>>>
>>>> Previously simplifyRegion did more than just ensuring that there is only
>>>> one entering and one exiting edge. It ensured that the entering block had no
>>>> other outgoing edge which was necessary for executeScopConditionally(). Now
>>>> the latter uses the alternative splitEdge implementation which can handle
>>>> this situation so simplifyRegion really only needs to simplify the region.
>>>>
>>>> Also, executeScopConditionally assumed that there can be no PHI nodes in
>>>> blocks with one incoming edge. This is wrong and LCSSA deliberately produces
>>>> such edges. However, previous passes ensured that there can be no such PHIs
>>>> in exit nodes, but which will no longer hold in the future.
>>>>
>>>> The new code that the property that it preserves the identity of region
>>>> block (the property that the memory address of the BasicBlock containing the
>>>> instructions remains the same; new blocks only contain PHI nodes and a
>>>> terminator), especially the entry block. As a result, there is no need to
>>>> update the reference to the BasicBlock of ScopStmt that contain its
>>>> instructions because they have been moved to other basic blocks.
>>>>
>>>> Reviewers: grosser
>>>>
>>>> Part of Differential Revision: http://reviews.llvm.org/D11867
>>>
>>>
>>> It seems this triggers two LNT failures:
>>>
>>> http://lab.llvm.org:8011/builders/perf-x86_64-penryn-O3-polly-fast/builds/10539
>>>
>>> Tobias