[PATCH] D16522: BlockGenerators: Replace getNewScalarValue with getNewValue

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

Thanks for committing. I will go an and commit my patches on top of it.


================
Comment at: lib/CodeGen/BlockGenerators.cpp:1216
@@ -1262,3 +1215,3 @@
 
-    Val = getNewScalarValue(Val, R, Stmt, LTS, *LocalBBMap);
+    Val = getNewValue(Stmt, Val, *LocalBBMap, LTS, L);
     Builder.CreateStore(Val, Address);
----------------
grosser wrote:
> Meinersbur wrote:
> > I see the problem with getExit(), but I am not yet convinced it is wrong. If it's the backedge that means that we did not leave that loop, i.e. passing that loop to the SCEV expander is correct.
> > 
> > On the other side, getEntry() might be wrong as well. With -polly-allow-nonaffine-loops, there might be a loop in the non-affine subregion as well and the SCEV depend on that. If the entry is part of the loop, SCEV expander will generate an expression depending on its induction variable, but the point of evaluation is the edge to the exit block, which is not part of the contained loop.
> > 
> > ASCII-Art:
> > ```
> >   _____
> >  /      Entry
> >  \      /       
> >   - loop       
> >        |               
> >   loop.after  
> >         |  \
> >         |   side
> >         |   /
> >         Exit
> > ```
> > Let's say the loop Entry=>loop has a non-affine exit condition (such as "i*i <n" which AFAIK is supported by SCEV). With -polly-allow-nonaffine-loops, the non-affine subregion should be Entry=>Exit. Exit has a phi:
> > 
> > Exit:
> >   %phi = phi i32 [%i.inc, %loop.after], [0, %side]
> > 
> > %i.inc needs to be evaluated in %loop.after for the MK_PHI MemoryAccess, but if passing the Loop Entry=>loop to it, we expand to an expression that depends on the loop's iv.
> > 
> Hi Michael,
> 
> for now I took your getExit() and committed the patch. I am not 100% certain this is the right choice, but have currently difficulties to write working test cases as the non-affine loop domain generation is broken (http://llvm.org/PR26309, http://llvm.org/PR26310). As this code does not yet seem to be tested, is not enabled by default and seems to have been broken (for this corner case) before, I don't think this should block this patch. Hence, I pushed this out in https://llvm.org/svn/llvm-project/polly/trunk@258799
> 
> I also opened a bug report to track this issue until the domain generation is fixed: http://PR26311
Hi Tobias,

given that loops within non-affine subregions are currently unsupported, I think it might have been safer to go with the getEntry() version because then we can assume there is no additional loop that the entry block is part of, but not all of the region.

Rethinking my argument, it is probably wrong too: we could enter a new loop (header) that is not part of the non-affine subregion.

What we are really looking for is the loop of the subregion's exiting edges. We could get it e.g.:
- Take the top entry of ScopStmt's NestLoops (if not empty)
- Get the innermost loop that contains both, the subregion exiting and exit node.


Repository:
  rL LLVM

http://reviews.llvm.org/D16522