[PATCH] D42833: [LICM] update BlockColors after splitting predecessors

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:860-863
+  // FIXME: it's not impossible to split LandingPad blocks, but it require
+  // updating BlockColors for all offspring blocks. By skipping such corner
+  // case, we can make updating BlockColors after splitting predecessor fairly
+  // simple.
----------------
majnemer wrote:
> junbuml wrote:
> > majnemer wrote:
> > > You don't need to worry about updating BlockColors if we have landingpads. If there is a landingpad, BlockColors should be entirely empty.
> > As far as I see, BlockColors is not empty when we have landingpads.  
> > For example, in the IR below, I can see the color for %lpadBB is %lpadBB and the color of its successor (%lpadBBSucc1) is %lpadBB, which is just what I expected after colorEHFunclets().  If we split %lpadBB for sinking, SplitLandingPadPredecessors() will be called and the new blocks split (%lpadBB.split) become landpad blocks. Then, color for new blocks and its all successors should be changed accordingly based the rule in colorEHFunclets().  Please correct me if I miss something.
> > 
> > ```
> >  define void @test(i1 %b, i32 %v1, i32 %v2) personality i32 (...)* @__CxxFrameHandler3 {
> > entry:
> >   br label %while.cond
> > while.cond:
> >   br i1 %b, label %try.cont, label %while.body
> > 
> > while.body:
> >   invoke void @may_throw()
> >           to label %while.body2 unwind label %lpadBB
> > 
> > while.body2:
> >   %v = call i32 @getv()
> >   %sinkable= mul i32 %v, %v2
> >   invoke void @may_throw2()
> >           to label %while.cond unwind label %lpadBB
> > lpadBB:
> >   %.lcssa1 = phi i32 [ 0, %while.body ], [ %sinkable, %while.body2 ]
> > 
> >   landingpad { i8*, i32 }
> >                catch i8* null
> >   br label %lpadBBSucc1
> > 
> > lpadBBSucc1:
> >   ret void
> > 
> > try.cont:
> >   ret void
> > }
> >  
> > ```
> >  
> Using _CxxFrameHandler3 with LandingPadInst is unsupported to the best of my knowledge.
> 
> Regardless, I think the best version of the predicate is:
>   if (!BlockColors.empty() && BB->getFirstNonPHI()->isEHPad())
> 
> This way we will still DTRT when using less interesting personality routines.
It make sense to me. I added a test case for this.
Thanks David. 


https://reviews.llvm.org/D42833