Page MenuHomePhabricator

[SimplifyCFG] avoid crash on degenerate loop
ClosedPublic

Authored by spatel on Thu, Jun 10, 2:30 PM.

Details

Summary

I'm not sure how to describe the code pattern in the test based on:
https://llvm.org/PR50638
...but if the IfCond is itself the phi that we are trying to remove, then the loop at line 2835 can end up with something like:

%cmp = select i1 %cmp, i1 false, i1 true

And that can then lead to an assert somewhere else (although I'm still not seeing that locally in my release + asserts build). I think this can only happen with unreachable code.

Diff Detail

Event Timeline

spatel created this revision.Thu, Jun 10, 2:30 PM
spatel requested review of this revision.Thu, Jun 10, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jun 10, 2:30 PM

And what happens when there's more than one PHI node in the block?
I think the extra missing check is !isa<Instruction>(IfCond) || cast<Instruction>(IfCond)->getParent() != BB.

lebedev.ri added inline comments.Thu, Jun 10, 2:53 PM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2710

(1) If you change IfCond's type to AssertingVH<Value> ...

2843

(3) ... so here on the second loop iteration IfCond is use-after-free.

2846

(2) ... the backtrace will tell you that the handle goes bad here ...

And what happens when there's more than one PHI node in the block?
I think the extra missing check is !isa<Instruction>(IfCond) || cast<Instruction>(IfCond)->getParent() != BB.

And thinking about it more, this should even be PHINode, not Instruction.

And what happens when there's more than one PHI node in the block?

If I'm seeing the code correctly, we can't get into trouble on anything other than the pattern in the test. That's because we limit the transform to a block with at most 2 phis (line 2723). If the problematic phi was 2nd, we'd escape without trying to touch the freed instruction.

But it's a valid concern (the assumption of 2 phis could change some day), so I agree, we should do better...

I think the extra missing check is !isa<Instruction>(IfCond) || cast<Instruction>(IfCond)->getParent() != BB.

But this won't work unless I've misunderstood the suggestion - the condition could be a function argument. Here's the first failing test that I found with that check:

define i32 @FoldTwoEntryPHINode(i1 %C, i32 %V1, i32 %V2, i16 %V3) {
entry:
        br i1 %C, label %then, label %else, !prof !0, !unpredictable !1
then:
        %V4 = or i32 %V2, %V1
        br label %Cont
else:
        %V5 = sext i16 %V3 to i32
        br label %Cont
Cont:
        %V6 = phi i32 [ %V5, %else ], [ %V4, %then ]
        call i32 @FoldTwoEntryPHINode( i1 false, i32 0, i32 0, i16 0 )
        ret i32 %V1
}

We still want that to fold to:

; CHECK-LABEL: @FoldTwoEntryPHINode(
; CHECK-NEXT:  entry:
; CHECK-NEXT:  %V5 = sext i16 %V3 to i32
; CHECK-NEXT:  %V4 = or i32 %V2, %V1
; CHECK-NEXT:  %V6 = select i1 %C, i32 %V4, i32 %V5, !prof !0, !unpredictable !1
; CHECK-NEXT:  %0 = call i32 @FoldTwoEntryPHINode(i1 false, i32 0, i32 0, i16 0)
; CHECK-NEXT:  ret i32 %V1
spatel updated this revision to Diff 351413.Fri, Jun 11, 5:14 AM

Patch updated:
Make the bailout more general and try to explain the problem pattern in a code comment.

lebedev.ri accepted this revision.Fri, Jun 11, 5:18 AM

Patch updated:
Make the bailout more general and try to explain the problem pattern in a code comment.

Thanks, that's precisely what i meant.
Thanks.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2718

I think we only need to care about PHI nodes

This revision is now accepted and ready to land.Fri, Jun 11, 5:18 AM
spatel marked an inline comment as done.Fri, Jun 11, 5:56 AM
spatel added inline comments.
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2718

Sounds good - I'll change that on final commit.

This revision was automatically updated to reflect the committed changes.
spatel marked an inline comment as done.