[polly] scop detection: could we stop iterating over uses?

[Tobias Grosser]

On 06/04/2013 10:54 PM, Tobias Grosser wrote:
> On 06/03/2013 09:22 AM, Sebastian Pop wrote:
>> Hi,
>>
>> I see that we call hasScalarDependency over every instruction in a
>> region, and
>> that iterates over all the uses of the instruction.  Here is the code
>> of that
>> function, with a nice "dirty hack" comment in it:
>>
>> bool ScopDetection::hasScalarDependency(Instruction &Inst,
>>                                          Region &RefRegion) const {
>>    for (Instruction::use_iterator UI = Inst.use_begin(), UE =
>> Inst.use_end();
>>         UI != UE; ++UI)
>>      if (Instruction *Use = dyn_cast<Instruction>(*UI))
>>        if (!RefRegion.contains(Use->getParent())) {
>>          // DirtyHack 1: PHINode user outside the Scop is not allow,
>> if this
>>          // PHINode is induction variable, the scalar to array
>> transform may
>>          // break it and introduce a non-indvar PHINode, which is not
>> allow in
>>          // Scop.
>>          // This can be fix by:
>>          // Introduce a IndependentBlockPrepare pass, which translate all
>>          // PHINodes not in Scop to array.
>>          // The IndependentBlockPrepare pass can also split the entry
>> block of
>>          // the function to hold the alloca instruction created by
>> scalar to
>>          // array.  and split the exit block of the Scop so the new
>> create load
>>          // instruction for escape users will not break other Scops.
>>          if (isa<PHINode>(Use))
>>            return true;
>>        }
>>
>>    return false;
>> }
>>
>> My question is how do we remove all this code: it is very expensive!
>
> 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 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.

Also, in terms of speeding up scop detection. My guess is that the 
largest speedup would come from detecting scops bottom up, starting from 
small regions to bigger regions. We could then skip analyzing regions as 
soon as we found a construct in a smaller region that would
also invalidate all regions containing the smaller region.

Cheers,
Tobi