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

[Johannes Doerfert via llvm-commits]

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.

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

> ================
> Comment at: include/polly/ScopInfo.h:1214
> @@ -1213,1 +1213,3 @@
> +  /// @see isIgnored()
> +  void simplifySCoP(bool RemoveIgnoredStmts);
>  
> ----------------
> jdoerfert wrote:
> > What is a no-op statement here? And why should we build ignored statements in the first place? Aren't they deleted anyway? Do you have performance numbers for this patch by any chance?
> > What is a no-op statement here? 
> 
> One without side-effect. Currently it checks whether there is no MemoryAccess at all but we could also remove those which have no write access.


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


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

> > Do you have performance numbers for this patch by any chance?
> 
> No. I don't expect a noticeable impact on performance.
> 
> ================
> Comment at: lib/Analysis/ScopInfo.cpp:2312
> @@ -2315,1 +2311,3 @@
> +  // Remove statments without accesses.
> +  simplifySCoP(false);
>  
> ----------------
> jdoerfert wrote:
> > Calling simplifySCoP 3 times seems a lot. Alone in this method we (directly or indirectly) iterate 3 __more__ times over all statements now.
> I removed one of them in the actual commit. Iterating over all statements is cheap.
> 
> ================
> Comment at: lib/Analysis/ScopInfo.cpp:2380
> @@ +2379,3 @@
> +
> +    if (StmtIt->isEmpty() || (RemoveIgnoredStmts && isIgnored(RN))) {
> +      // Remove the statement because it is unnecessary.
> ----------------
> jdoerfert wrote:
> > Please try to keep/write early exit code instead of indention.
> OK
> 
> ================
> Comment at: lib/Analysis/ScopInfo.cpp:2898
> @@ +2897,3 @@
> +      Domain = isl_set_empty(isl_space_set_alloc(IslCtx, 0, 0));
> +    }
> +    auto *UDomain = isl_union_set_from_set(Domain);
> ----------------
> jdoerfert wrote:
> > Meinersbur wrote:
> > > A test case fails without it. We might instead bail out early as invalid when we encounter a Scop without statements.
> > Which test? Unit or lnt? Shouldn't we fix this somehow? Generating an empyt domain for the case there is no statement seems rather odd.
> I think it was whole-scop-non-affine-subregion-in-loop.ll
> 
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151002/e0db61f8/attachment.sig>