[polly] r183799 - scop detection: remove an iteration over all uses

[Tobias Grosser]

On 06/14/2013 01:05 PM, Sebastian Pop wrote:
> Tobias Grosser wrote:
>> On 06/11/2013 03:20 PM, Sebastian Pop wrote:
>>> Author: spop
>>> Date: Tue Jun 11 17:20:35 2013
>>> New Revision: 183799
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=183799&view=rev
>>> Log:
>>> scop detection: remove an iteration over all uses
>>
>> This one breaks the attached test case if called with:
>>
>> $ polly-opt /tmp/dependency_to_phi_node_outside_of_region.ll
>>
>> I thought I posted this in reply to the patch you posted for review, no?
>
> I remember having seen this testcase, however I have not got it from your
> message that you implied that this testcase would fail:
>
> You wrote:
>> I just checked and there is no test case in the polly test cases that
>> would fail if we remove this function. However, I was able to create a
>> test case that is similar to the description in the comment.
>>
>>  From a first view I do not really see why PHI nodes need to be handled
>> in a special way. In fact, when removing this function as well as the
>> two asserts in the IndependentBlocks pass, the attached test case is
>> correctly code generated.
>
> I interpreted this as: the testcase was correctly compiled with the patch.

Only if you remove the two asserts in the IndependentBlocks pass.

>> I can see that there may be some problems
>> with invalidating later scops when translating out of SSA, but I do
>> not see how this is specific to PHI nodes. So from my point of view, I
>> would
>> be OK with removing this code (and add the relevant test cases to
>> ensure we do the right thing) given such a patch passes the LLVM test
>> suite without regressions.

I OKed this commit, given it removes the asserts and it passes the LLVM 
test-suite without regressions. Did you run this on the LLVM test-suite?
Did you see any regressions in your internal testing?

>> However, maybe ether has some more insights. I remember he was the one
>> adding this code in, but I don't remember the exact reasoning behind
>> this.
>>
>
> And Ether said:
>> What I remember is there are some fail cases when we running polly on the
>> testsuite without this function...
>>
>> At that time If an instruction is used in another SCoP, the independent
>> blocks pass will generate some mess to make the  induction variable become
>> a non- induction variable, and this non- induction variable PHINode will
>> confuse the later passes.
>> I think the commit "Remove dependence on canonical induction variable"
>> which introduce canSynthesize in line 310 of the IndependentBlocks.cpp
>> implicitly fix this.
>
> So I thought it was ok to commit the patch.
>
> Do you want me to revert the patch until we figure out why the testcase fails?

If the patch is tested and does not cause any regressions on the LLVM 
test suite, please just remove the asserts and commit the test case. If 
this check still causes regressions on the test suite or was never 
tested on it, please remove it until you find time to test it.

Cheers,
Tobias