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

[Sebastian Pop]

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.

> 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.
> 
> 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?

Thanks,
Sebastian
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation