[PATCH] D77560: [SCEV] CreateNodeForPhi should return SCEVUnknown for incomplete PHIs

[wael yehia via Phabricator via llvm-commits]

w2yehia created this revision.
Herald added subscribers: llvm-commits, steven.zhang, javed.absar, hiraditya.
Herald added a project: LLVM.

The problem
-----------

SCEVExpander::getAddRecExprPHILiterally might call itself recursively (when calling expandIVInc), and in the process attempts to create a SCEV for the PHINode it is constructing. The resulting SCEV would never be correct. 
ScalaraEvolution caches SCEVs in `ValueExprMap`, and ScalarEvolution.getSCEV uses that cache.

The issue is that two different PHIs, might look the same when they are in an incomplete state, for example:

  %incomplete_1 = phi i32 [0, %entry]   ;; missing incoming value from the latch block
  %incomplete_2 = phi i32 [0, %entry]   ;; missing incoming value from the latch block

and so createSCEV would return the same, incorrect, SCEV value.

Users of ScalarEvolution, such as Loop Strength Reduction, assume that two Values that have the same SCEV are "equivalent".

Solution 1 (this patch):
------------------------

ScalarEvolution::createSCEV returns SCEVUnknown for incomplete SCEVs

Solution 2 (not implemented):
-----------------------------

Have SCEVExpander::getAddRecExprPHILiterally delete the old entry in `ValueExprMap` when a new incoming value is added to the PHI that is under construction.
Actually, any other data structure that stores the "incorrect" SCEV should prune it. Which leads to thinking that some sort of call back mechanism has to be used when an incoming value is added to the PHI. I couldn't find such a call back. `CallBackVH` only deals with deleting and replace a `Value`

Testcase:
---------

I have the following reduced testcase from bugpoint, but I don't see how useful it is or to how make it useful:

  source_filename = "t.cpp"
  target datalayout = "e-m:e-i64:64-n32:64"
  target triple = "powerpc64le-unknown-linux-gnu"
  
  define dso_local void @foo(i32* %.n) local_unnamed_addr {
  foo_entry:
    %n = load i32, i32* %.n, align 4
    %_conv19 = sext i32 %n to i64
    br label %_loop_4_do_
  
  _loop_4_do_:                                      ; preds = %_loop_7_endl_, %foo_entry
    %lda.0304 = phi i64 [ undef, %foo_entry ], [ %_add_tmp68, %_loop_7_endl_ ]
    %ib.0302 = phi i64 [ 0, %foo_entry ], [ %_add_tmp84, %_loop_7_endl_ ]
    %_add_tmp66 = add nsw i64 %lda.0304, %_conv19
    %_add_tmp68 = add nsw i64 %_add_tmp66, %_conv19
    %_mult_tmp70.neg = add i64 %ib.0302, 2
    %_sub_tmp71 = sub i64 %_mult_tmp70.neg, undef
    %_add_tmp74 = add i64 %_add_tmp66, undef
    %_mult_tmp75 = shl i64 %_add_tmp74, 1
    %_add_tmp76 = add nsw i64 %_sub_tmp71, %_mult_tmp75
    %_add_tmp84 = add nsw i64 %_add_tmp76, 32
    br i1 undef, label %_loop_7_endl_, label %_loop_7_do_
  
  _loop_7_do_:                                      ; preds = %_loop_7_do_, %_loop_4_do_
    %i.0289 = phi i64 [ %_add_tmp678.3, %_loop_7_do_ ], [ undef, %_loop_4_do_ ]
    %_add_tmp678.2 = add nuw nsw i64 %i.0289, 3
    %_mult_tmp619.3 = shl nuw i64 %_add_tmp678.2, 1
    %_add_tmp622.3199 = add i64 %_mult_tmp619.3, %_add_tmp76
    %0 = shl i64 %_add_tmp622.3199, 2
    %uglygep200 = getelementptr i8, i8* undef, i64 %0
    %uglygep201 = getelementptr i8, i8* %uglygep200, i64 -4
    %1 = bitcast i8* %uglygep201 to float*
    %_add_tmp678.3 = add nuw nsw i64 %i.0289, 4
    %_add_tmp724.3215 = add i64 %_mult_tmp619.3, %_sub_tmp71
    %2 = shl i64 %_add_tmp724.3215, 2
    %uglygep216 = getelementptr i8, i8* undef, i64 %2
    %uglygep217 = getelementptr i8, i8* %uglygep216, i64 -4
    %3 = bitcast i8* %uglygep217 to float*
    store float undef, float* %3, align 4
    %4 = call i64 @bar(i64 undef, float* nonnull %1, i64 4, i64 4)
    br label %_loop_7_do_
  
  _loop_7_endl_:                                    ; preds = %_loop_4_do_
    br label %_loop_4_do_
  }
  declare i64 @bar(i64, float*, i64, i64) local_unnamed_addr




Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77560

Files:
  llvm/lib/Analysis/ScalarEvolution.cpp


Index: llvm/lib/Analysis/ScalarEvolution.cpp
===================================================================
--- llvm/lib/Analysis/ScalarEvolution.cpp
+++ llvm/lib/Analysis/ScalarEvolution.cpp
@@ -5359,6 +5359,13 @@
 }
 
 const SCEV *ScalarEvolution::createNodeForPHI(PHINode *PN) {
+  // If the PHI is in the midst of being constructed, it does not make sense to
+  // compute a SCEV for it. Here we assume that for every predecessor block
+  // there is a corresponding incoming value in the PHI.
+  unsigned NumIncoming = PN->getNumIncomingValues();
+  if (!PN->getParent()->hasNPredecessors(NumIncoming))
+    return getUnknown(PN);
+
   if (const SCEV *S = createAddRecFromPHI(PN))
     return S;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77560.255329.patch
Type: text/x-patch
Size: 708 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200406/2610025a/attachment.bin>