Page MenuHomePhabricator

[ScalarEvolution] Add bailout to avoid zext of pointer.
AcceptedPublic

Authored by efriedma on Wed, Nov 24, 11:22 AM.

Details

Summary

The RHS of an isImpliedCond call can be a pointer even if the LHS is not. This is similar to bfa2a81e.

Not going to include a testcase; an IR testcase would be extremely complicated and fragile.

Fixes https://bugs.llvm.org/show_bug.cgi?id=52594 .

Diff Detail

Event Timeline

efriedma created this revision.Wed, Nov 24, 11:22 AM
efriedma requested review of this revision.Wed, Nov 24, 11:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Nov 24, 11:22 AM
emaste added a subscriber: emaste.Wed, Nov 24, 11:27 AM
dim added a comment.Thu, Nov 25, 1:06 AM

FWIW, this fixes both the minimized test case from PR52594, and the full original test case from https://bugs.freebsd.org/260000.

fhahn added a comment.Thu, Nov 25, 2:16 AM

I'm wondering about:

Not going to include a testcase; an IR testcase would be extremely complicated and fragile.

Below a reduced version from PR52594, which doesn't look too bad IMO. Is there anything in particular you think makes this test very fragile>

target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"

define void @test(i8* %a, i8* %b, i64 %c) {
entry:
  br label %loop.1.header

loop.1.header:
  %iv.1 = phi i8* [ %a, %entry ], [ %iv.1.next, %loop.1.latch ]
  %cond = icmp eq i64 %c, 0
  br i1 %cond, label %loop.1.latch, label %exit

loop.1.latch:
  %iv.1.next = getelementptr inbounds i8, i8* %iv.1, i32 1
  %ec = icmp eq i8* %iv.1.next, %b
  br i1 %ec, label %loop.2.ph, label %loop.1.header

loop.2.ph:
  br label %loop.2

loop.2:
  %iv.2 = phi i8* [ %iv.1.next, %loop.2.ph ], [ %iv.2.next, %loop.2 ]
  %iv.2.int = ptrtoint i8* %iv.2 to i32
  call void @wobble(i32 %iv.2.int)
  %ec.1 = icmp ult i8* %iv.2, %b
  %ec.1.trunc = and i1 %ec.1, 1
  %iv.2.next = getelementptr inbounds i8, i8* %iv.2, i32 1
  br i1 %ec.1.trunc, label %loop.2, label %exit

exit:
  ret void
}

declare void @wobble(i32)
llvm/lib/Analysis/ScalarEvolution.cpp
10789

can this be moved up before if (!CmpInst::isSigned(FoundPred).... and the pointer check dropped from the condition above?

mkazantsev added inline comments.Sun, Nov 28, 8:52 PM
llvm/lib/Analysis/ScalarEvolution.cpp
10800

This doesn't make sense to me. I'd expect that both FoundLHS and FoundRHS should be of the same type. How come they are different? Isn't it the real reason of the bug?

The testcase isn't that complicated in terms of the size of the IR, but it would be hard to ensure it continues to test the codepath in question. The actual crash involves many levels of recursive calls in SCEV, and we aren't expecting any useful information out of the isImpliedCond analysis in this case. I could include it anyway, I guess.

llvm/lib/Analysis/ScalarEvolution.cpp
10800

Like we found doing the review for bfa2a81e, the pointer-ness of the LHS/RHS should match for an icmp instruction, but ScalarEvolution makes other calls recursively. See, for example, ScalarEvolution::isImpliedCondOperandsHelper.

At one point, I was considering using ptrtoint on all pointer inputs to isImpliedCond, but that turned out to be a little more complicated than I expected, and not really necessary. I could revive that, I guess.

efriedma added inline comments.Mon, Nov 29, 12:13 AM
llvm/lib/Analysis/ScalarEvolution.cpp
10789

This check is checking FoundLHS/FoundRHS; the if (!CmpInst::isSigned(FoundPred).... check is checking LHS/RHS.

reames accepted this revision.Mon, Nov 29, 12:13 PM

LGTM

This revision is now accepted and ready to land.Mon, Nov 29, 12:13 PM