[PATCH] D16522: BlockGenerators: Replace getNewScalarValue with getNewValue

[Tobias Grosser via llvm-commits]

grosser added inline comments.

================
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);
----------------
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


Repository:
  rL LLVM

http://reviews.llvm.org/D16522