[PATCH] D16522: BlockGenerators: Replace getNewScalarValue with getNewValue

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Michael,

thanks for the comments. I added your test case and addressed the PHI/EXITING_PHI issue. Could you confirm I did not miss anything and this is now ready to go?

Best,
Tobias


================
Comment at: lib/CodeGen/BlockGenerators.cpp:1222
@@ -1262,2 +1221,3 @@
 
-    Val = getNewScalarValue(Val, R, Stmt, LTS, *LocalBBMap);
+    Loop *L = getLoopForInst(ScalarInst);
+    Val = getNewValue(Stmt, Val, *LocalBBMap, LTS, L);
----------------
Meinersbur wrote:
> grosser wrote:
> > Meinersbur wrote:
> > > This is wrong; we don't need the loop where the value is defined but where it is used. The difference comes in to play when the value is defined in the loop, but we write its scalar value after the loop, thus use the value after the last loop iteration.
> > Is this not what is happening? ScalarInst comes from getAccessInstruction(), which is defined to return:
> > 
> > ```
> >   /// For memory accesses of kind MK_Value the access instruction of a load      
> >   /// access is the instruction that uses the load. The access instruction of    
> >   /// a write access is the instruction that defines the llvm::Value.
> > ```
> > 
> > Now, the original code was slightly different. It was using: getLoopForInst(ScalarValueInst) where ScalarValueInst was the definition of the ScalarValueInst.
> > 
> > So it seems the behavior was changed (maybe needs a test case?), but the result should be more correct? In case I got it wrong, could you suggest what value to use otherwise.
> > 
> > (In case we drop the MA->getAccessInstruction() mapping for scalar values, we can probably just use the loop around the entry block.)
> This is true for accesses of type MK_Value (where the definition must necessarily be in the same Stmt), but not for MK_PHI or MK_ExitPHI. Here, getAccessInstruction() returns the incoming value (at least before D15681) which might be defined anywhere before. Eg.
> ```
> loop:
>   %i = phi i32 [ 0, %entry ], [ %i.inc, %loop ]
>   %i.inc = add nsw i32 %i, 1
>   %cmp5 = icmp slt i32 %i.inc, 2
>   br i1 %cmp5, label %exit, label %loop
> 
> exit:
>   br label %join
> 
> join:
>   %phi = phi i32 [i.inc, %join]
> ```
> There'd be a PHI write in Stmt_exit, writing the value of %i.inc. Hence, the AccessInstruction of that would be %i.inc. Synthesizing it within loop (getLoopForInt(%i.inc)) will expand to something dependent on %i, whereas at %exit the correct value is 2.
> 
> You might look into my version of the patch for how I solved the issue.
I looked at your code and now derive the loop from getRegion->getEntry().

You derive it from getExit(). This might be incorrect if the region is on a backedge and the exit is the header of a loop that dominates the original region. This is highly unlikely, though. getEntry() on the other hand seems to be save in general as it is itself within the scop region.


http://reviews.llvm.org/D16522