[PATCH] D13341: [Polly] Earlier creation of ScopStmt objects

[Michael Kruse via llvm-commits]

2015-10-02 17:12 GMT+02:00 Johannes Doerfert <doerfert at cs.uni-saarland.de>:
> On 10/02, Michael Kruse wrote:
>> Meinersbur added a comment.
>>
>> In http://reviews.llvm.org/D13341#258594, @jdoerfert wrote:
>>
>> > Why don't we create statements (and accesses) during the domain generation [second part, propagation]?
>>
>>
>> For DeLICM, while adding MemoryAccesses, there will be a multiple per-Stmt data for which I'd introduce new dictionaries (and lookups) from BasicBlocks to that data. We save this by just using the already existing StmtMap. Problem was, those was created only later.
> I think we should talk next Wednesday about the DeLICM stuff again. It
> seems to be rather complex and we might be able to come up with a
> simpler way.

I described the algorithm a while ago. You can also look at
http://reviews.llvm.org/D12975 for an incomplete implementation.
Teobias and I discussed it in the phone call last week.


>> In http://reviews.llvm.org/D13341#258594, @jdoerfert wrote:
>>
>> > Con:
>> >
>> > - Domain generation would perform another task (though, so far schedule generation did this and it seemed fine).
>>
>>
>> This is an important point (but not the reason for this patch). Doing multiple unrelated things in one function makes code hard to understand and modify (e.g. changing the order of the two operations).
> Agreed, but generating empty statements (that can probably not even be
> dumped) until at some point ScopStmt::init is called seems not like an
> improvement to me. Additionally, we have to keep an eye on compile time
> and overhead. Iterating X times over the SCoP separates things nicely,
> but it might also be costly.

In the long run, it decreases compile time because there are fewer map
lookups to do.

The typical number of stmts in a scop is single digit. Nothing to worry about.


>
>> > And why should we build ignored statements in the first place?
>>
>> Because we don't know yet whether they are ignored. E.g. because of load hoisting.
> That is not true. Scop::isIgnored() checks for three things that are not affected by load hoisting.

simplySCoP does remove statements which we didn't know to remove
before load hosting. This is not specific to isIgnored()


>
>> > Aren't they deleted anyway?
>>
>> At the last simplifyScop, yes; but we don't need to do the calculations in-between when we already know they are going to be removed.
> Isn't that my argument? Why don't need them if we know they are going to
> be removed. But why are they created?

My argument is to remove them as early as possible.

I would agree that creating those wouldn't be necessary if there was
no use for those ScopStmts before building domains. We could already
collapse the DomainMap into StmtMap and have one less dictionary and
only one location to store domains.



>> This is by design. The affine region is removed because there are no accesses in it. That leaves no statements in the scop. How would you handle this?
> isIgnored() should return true for such a region if it doesn't have
> accesses in the first place. If it had but not anymore and it was the
> last/only statement in the SCoP we should bail.

OK. I will implement a bail-out.

Michael