Page MenuHomePhabricator

Handling null AssumptionCache in simplifyCFG
ClosedPublic

Authored by rcorcs on Nov 7 2019, 12:57 PM.

Details

Summary

AssumptionCache can be null in SimplifyCFGOptions. However, FoldCondBranchOnPHI() was not properly handling that when passing a null AssumptionCache to simplifyCFG.

Diff Detail

Event Timeline

rcorcs created this revision.Nov 7 2019, 12:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2019, 12:57 PM
rcorcs updated this revision to Diff 228340.Nov 7 2019, 4:50 PM

Added test case that exercises this issue.

rcorcs updated this revision to Diff 228345.Nov 7 2019, 5:33 PM

Simplified test case.

LGTM with a few small suggestions. Please wait a bit with committing in case there are additional comments.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2276–2277

While you are at it, it might be worth simplifying the condition to something like if (AC && match(N, m_Intrinsic<IntrinsicInst::assume>())

llvm/unittests/Transforms/Utils/LocalTest.cpp
992

might be worth stating that this is just to get a pointer to %test.bb. You could also just check the BB name, which is slightly more direct. Also, it would be good to assert that we found TestBB.

999

nit: you could just use EXPECT_TRUE directly, I do not think the variable adds much.

spatel accepted this revision.Dec 4 2019, 8:46 AM

LGTM with the changes suggested.

This revision is now accepted and ready to land.Dec 4 2019, 8:46 AM
rcorcs updated this revision to Diff 232699.Dec 7 2019, 7:25 AM

Addressed comments.

Thanks everyone for the review.

Can someone please commit this patch for me?

fhahn added a comment.Dec 7 2019, 8:43 AM

Thanks, I'll land the patch

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2276–2277

This is not required the using match as below. I'll update it before committing

This revision was automatically updated to reflect the committed changes.
xbolva00 added inline comments.
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2277

if (AC && N && match(N, m_Intrinsic<Intrinsic::assume>()))

?