[PATCH] D10903: [LICM] Replace incoming values with undef if the incoming BB is unreachable

[hfinkel at anl.gov]

hfinkel accepted this revision.
hfinkel added a reviewer: hfinkel.
This revision is now accepted and ready to land.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:614-621
@@ -612,1 +613,10 @@
 
+    // Replace incoming values with undef if the incoming BB is unreachable.
+    Use &U = UI.getUse();
+    BasicBlock *BB = PN->getIncomingBlock(U);
+    if (!DT->isReachableFromEntry(BB)) {
+      // Replace the use with an undef value.
+      U = UndefValue::get(I.getType());
+      continue;
+    }
+
----------------
majnemer wrote:
> hfinkel wrote:
> > majnemer wrote:
> > > chandlerc wrote:
> > > > Why do this here rather than in InstSimplify? Do we form these too late for that to clean it up? If so, should we add it to InstSimplify and call that here?
> > > I don't know what we could do in InstructionSimplify instead of here.
> > > 
> > > Running InstructionSimplify on the PHI won't help because it cannot create new instructions, it wouldn't be able to give us a new PHI which had the incoming value replaced with undef.
> > > 
> > > Running InstructionSimplify on the incoming value won't help because it might not be simplifiable.  My trivial test case has a trivial incoming value but we cannot, in the general case, rely on simplification yielding a `Constant`.  For example:
> > >   define i32 @f(i1 zeroext %p1) {
> > >   entry:
> > >     br label %lbl
> > > 
> > >   lbl.loopexit:                                     ; No predecessors!
> > >     br label %lbl
> > > 
> > >   lbl:                                              ; preds = %lbl.loopexit, %entry
> > >     %phi = phi i32 [ %conv, %lbl.loopexit ], [ undef, %entry ]
> > >     br i1 %p1, label %if.then.5, label %out
> > > 
> > >   if.then.5:                                        ; preds = %if.then.5, %lbl
> > >     %conv = zext i1 %p1 to i32
> > >     br label %if.then.5
> > > 
> > >   out:                                              ; preds = %lbl
> > >     ret i32 %phi
> > >   }
> > InstCombine always has a DT now and can create new instructions, why not add this there?
> > 
> This fix address a miscompile, not a missed optimization.  Sorry for the miscommunication, I'll update the summary to have an in-depth description of the bug and the rationale behind the fix.
Okay; I'd rather see a comment in the code explaining why what it is doing is a necessary part of the algorithm.

Assuming you add such a comment, LGTM.



http://reviews.llvm.org/D10903