This is an archive of the discontinued LLVM Phabricator instance.

[LICM] update BlockColors after splitting predecessors
ClosedPublic

Authored by junbuml on Feb 1 2018, 10:50 PM.

Details

Summary

Update BlockColors after splitting predecessors. Do not allow splitting
LandingPad for sinking when the BlockColors is not empty, so we can
simply assign predecessor's color to the new block.

Fixes PR36184

Diff Detail

Event Timeline

junbuml created this revision.Feb 1 2018, 10:50 PM
junbuml updated this revision to Diff 132630.Feb 2 2018, 10:42 AM

Added a test case.

majnemer added inline comments.Feb 2 2018, 2:39 PM
lib/Transforms/Scalar/LICM.cpp
860–867

You don't need to worry about updating BlockColors if we have landingpads. If there is a landingpad, BlockColors should be entirely empty.

junbuml added inline comments.Feb 2 2018, 8:55 PM
lib/Transforms/Scalar/LICM.cpp
860–867

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
}
majnemer added inline comments.Feb 3 2018, 12:44 AM
lib/Transforms/Scalar/LICM.cpp
860–867

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.

junbuml updated this revision to Diff 132750.Feb 3 2018, 12:43 PM

Incorporated David comment.

lib/Transforms/Scalar/LICM.cpp
860–867

It make sense to me. I added a test case for this.
Thanks David.

junbuml updated this revision to Diff 132859.Feb 5 2018, 10:46 AM
junbuml edited the summary of this revision. (Show Details)

Minor changes in comments. Please let me know any further comment.

Kindly ping.
Sorry for the rush, but this is one of release-6.0 blockers.

Kindly ping.

Since this is one of release-6.0 blockers, I want to kindly ping one more time. After fixing @majnemer's comment, I believe this is pretty good shape now.
Just in case @majnemer is unavailable for reviewing this soon, added @hfinkel and @mcrosier as reviewer.

majnemer accepted this revision.Feb 12 2018, 8:23 AM

LGTM

lib/Transforms/Scalar/LICM.cpp
862–865

I'd remove the FIXME and replace LandingPad with EHPad

This revision is now accepted and ready to land.Feb 12 2018, 8:23 AM

Thanks David for the review.
Committed in r324916.

junbuml closed this revision.Feb 12 2018, 10:01 AM