[PATCH] D13611: [Polly] Create virtual independent blocks

Johannes Doerfert via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 15 17:09:43 PDT 2015


jdoerfert added a comment.

In http://reviews.llvm.org/D13611#268497, @Meinersbur wrote:

> In http://reviews.llvm.org/D13611#268449, @grosser wrote:
>
> > Hi Johannes,
> >
> > great to see we will finally get rid of the IndependentBlocks pass. I just had a very first skim over this patch, but did not manage to do a full review yet.
> >
> > The two main items I would still like to look into:
> >
> > 1. This seems to conflict with http://reviews.llvm.org/D12975 (Michael's DE-LICM)
> >
> >   I need to get a better understanding in which ways these patches possibly conflict (as well as the overall design).
>
>
> To add some details:
>
> http://reviews.llvm.org/D12975 tracks which values flow between ScopStmts which this patch modifies at a very late phase. There should be fewer such implicit flows, but some of those flows might have already be created with origin MAPPED. The array element occupied by this MAPPED flow might be used for some other implicit flow, meaning De-LICM before virtual blocks is ineffective. It is also more difficult to modify MAPPEDs because lifetime issues have to be considered and there is no distinction between SCALAR and PHI origins (not sure how much of a problem this is though).


See the comment on

> One of my rationale to do De-LICM before even creating MemoryAccesses is that it is easier to implement (no moving, copying, deleting of MemoryAccesses) and reduced overhead because fewer MemoryAccesses are created in the first place.


While moving, copying and deleting MemoryAccesses sound like a lot, one has to remember the number of times this can happen and the cost of each operation.
I think the fact that we get generally better results with this patch than without (see commit message) shows that the "overhead" is low enough.

The implementation difficulty is another issue. While this patch currently only forwards/copies scalars its general structure allows more, e.g., reload values instead of communicate them (see subsequent patch) or remove conditional PHI nodes and replace them by selects. While e.g., reloading is generally hard and costly on the CFG level (one has to determine if the value has been overwritten __somewhere__ in between) the SCoP abstraction allows to reason about it in a simple and consise way.

> So we have the possibilities to either do "simplifyScalarAccesses" before creating MemoryAccesses (e.g. by marking Instructions to where they should be copied) or "De-LICM" after simplifyScalarAccesses.


I already proposed the latter in a review last week but I don't recall a positive answer to that.


================
Comment at: lib/Analysis/ScopInfo.cpp:1383
@@ +1382,3 @@
+
+    while (OnlyMA && MALIt != MALEnd && (MA != *MALIt)) {
+      MALLastIt++;
----------------
Meinersbur wrote:
> I do not understand this code. Why did you even remove any comment about what it is doing?
This code was basically moved and the original comment was left behind, hence the answer to your question is simple:
  The comment has to be moved too.

================
Comment at: lib/Analysis/ScopInfo.cpp:2696
@@ +2695,3 @@
+
+void Scop::simplifyScalarAccesses() {
+  using OutsideOperandsSetTy =
----------------
Meinersbur wrote:
> God function antipattern
I take this comment should read as:
  Please split the function in three parts.
Sure, can do.


http://reviews.llvm.org/D13611





More information about the llvm-commits mailing list