[llvm] r347934 - [SCEV] Guard movement of insertion point for loop-invariants
David Jones via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 5 15:17:26 PST 2018
On Wed, Dec 5, 2018 at 2:56 PM <warren.ristow at sony.com> wrote:
> Hi David,
>
>
>
> [+ Reid]
>
>
>
> > ...
>
> > I suspect your change is exposing a pre-existing bug; but unfortunately,
> it does very
>
> > cleanly resolve to exactly r347934, and it occurs with multiple targets.
>
> >
>
> > Could you advise on a fix? Unfortunately, I'm not familiar with this
> code, so the best
>
> > option I can offer is a revert. (If you won't able to author a timely
> fix, please let
>
> > me know so I can submit the revert for you.)
>
>
>
> Sorry to hear about that. I definitely won't be able to author a timely
> fix, so please do go ahead and revert it.
>
>
OK... sorry for bringing out the big hammer. :-/
FYI: Committed revision 348426.
>
>
> As it happens, while looking at the history of this SCEV div-by-0 area, in
> prep for a follow-up patch:
>
> https://reviews.llvm.org/D55232
>
>
>
> I noticed that an equivalent change was made about a year ago (r289412).
> Reid found it caused a problem in instcombine when ASan was enabled. This
> issue you reported sounds very similar. Reid reverted that year-old commit
> at r289453:
>
>
>
> ‘’’
>
> Reverts r289412. It caused an OOB PHI operand access in instcombine when
>
> ASan is enabled. Reduction in progress.
>
> ‘’’
>
>
>
> > I don't feel I have a useful reproducer to share... between creduce and
> bugpoint, I've
>
> > gotten one target down to 68k of .bc (which disassembles to about 180k
> of .ll). (And it
>
> > would take a lot of effort to redact before I can share it.)
>
>
>
> Possibly Reid has access to a reduced test-case from back then (or can
> create one now)?
>
>
>
> I'm not familiar with the code where the instcombine failure is
> happening. Without a test-case, I feel my hands are tied. So a reduced
> test-case from either you or Reid would be greatly appreciated!
>
>
>
OK, in that case I will keep working on a reduced test case. Once I have
something I can share, I will forward to you.
Sorry about the revert, and thanks for your help.
--dlj
> Thanks,
>
> -Warren
>
>
>
> *From:* David Jones <dlj at google.com>
> *Sent:* Wednesday, December 5, 2018 2:25 PM
> *To:* Ristow, Warren <warren.ristow at sony.com>
> *Cc:* llvm-commits at lists.llvm.org
> *Subject:* Re: [llvm] r347934 - [SCEV] Guard movement of insertion point
> for loop-invariants
>
>
>
> Hi Warren --
>
>
>
> I've observed several Clang crashes on SEGV, which bisect to this change.
>
>
>
> The crashes generally look something like this:
>
>
>
> $ opt -O2 -o /dev/null bugpoint-reduced-simplified.ll
>
> Stack dump:
>
> 0. Program arguments: opt -O2 -o /dev/null
> bugpoint-reduced-simplified.ll
>
> 1. Running pass 'CallGraph Pass Manager' on module
> 'bugpoint-reduced-simplified.ll'.
>
> 2. Running pass 'Combine redundant instructions' on function
> '@_Z[redacted]'
>
> #0 0x0000558aa2221628 llvm::sys::RunSignalHandlers() (opt+0x1f3b628)
>
> #1 0x0000558aa2223746 SignalHandler(int) (opt+0x1f3d746)
>
> #2 0x00007f5def60a9a0 __restore_rt
> (/usr/grte/v4/lib64/libpthread.so.0+0xf9a0)
>
> #3 0x0000558aa1bbec7f llvm::InstCombiner::visitPHINode(llvm::PHINode&)
> (opt+0x18d8c7f)
>
> #4 0x0000558aa1b8db24 llvm::InstCombiner::run() (opt+0x18a7b24)
>
> #5 0x0000558aa1b8f504 combineInstructionsOverFunction(llvm::Function&,
> llvm::InstCombineWorklist&, llvm::AAResults*, llvm::AssumptionCache&,
> llvm::TargetLibraryInfo&, llvm::DominatorTree&,
> llvm::OptimizationRemarkEmitter&, bool, llvm::LoopInfo*) (opt+0x18a9504)
>
> #6 0x0000558aa1b8f8af
> llvm::InstructionCombiningPass::runOnFunction(llvm::Function&)
> (opt+0x18a98af)
>
> #7 0x0000558aa207d2fc llvm::FPPassManager::runOnFunction(llvm::Function&)
> (opt+0x1d972fc)
>
> #8 0x0000558aa1f062e0 (anonymous
> namespace)::CGPassManager::runOnModule(llvm::Module&) (opt+0x1c202e0)
>
> #9 0x0000558aa207da85 llvm::legacy::PassManagerImpl::run(llvm::Module&)
> (opt+0x1d97a85)
>
> #10 0x0000558aa0a8900b main (opt+0x7a300b)
>
> #11 0x00007f5def378bbd __libc_start_main
> (/usr/grte/v4/lib64/libc.so.6+0x38bbd)
>
> #12 0x0000558aa0a740e9 _start
> /usr/grte/v4/debug-src/src/csu/../sysdeps/x86_64/start.S:121:0
>
> [1] 17713 segmentation fault opt -O2 -o /dev/null
> bugpoint-reduced-simplified.ll
>
>
>
>
>
> gdb's backtrace is a bit more useful:
>
>
>
> #0 0x000055555958b1d4 in llvm::Use::get (this=<optimized out>) at
> third_party/llvm/llvm/include/llvm/IR/Use.h:108
>
> #1 llvm::PHINode::getOperand (this=<optimized out>,
> i_nocapture=4294967295) at
> third_party/llvm/llvm/include/llvm/IR/Instructions.h:2714
>
> #2 llvm::PHINode::getIncomingValue (this=<optimized out>, i=4294967295)
> at third_party/llvm/llvm/include/llvm/IR/Instructions.h:2603
>
> #3 0x000055555c41abba in llvm::InstCombiner::visitPHINode
> (this=0x7ffff4e3b970, PN=...) at
> third_party/llvm/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1238
>
> #4 0x000055555c378489 in llvm::InstCombiner::run (this=<optimized out>)
> at
> third_party/llvm/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3192
>
> #5 0x000055555c37b6d2 in combineInstructionsOverFunction (F=...,
> Worklist=..., AA=0x60600002b7c0, AC=..., TLI=..., DT=..., ORE=...,
> ExpensiveCombines=<optimized out>, LI=<optimized out>)
>
> at
> third_party/llvm/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3424
>
> #6 0x000055555c37be37 in llvm::InstructionCombiningPass::runOnFunction
> (this=<optimized out>, F=...) at
> third_party/llvm/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3483
>
> #7 0x000055555d39b609 in llvm::FPPassManager::runOnFunction
> (this=<optimized out>, F=...) at
> third_party/llvm/llvm/lib/IR/LegacyPassManager.cpp:1644
>
> #8 0x000055555ce8f11b in (anonymous
> namespace)::CGPassManager::RunPassOnSCC (this=<optimized out>,
> P=0x614000009a40, CurSCC=..., CG=..., CallGraphUpToDate=<optimized out>,
> DevirtualizedCall=<optimized out>)
>
> at third_party/llvm/llvm/lib/Analysis/CallGraphSCCPass.cpp:178
>
> #9 (anonymous namespace)::CGPassManager::RunAllPassesOnSCC
> (this=<optimized out>, CurSCC=..., CG=..., DevirtualizedCall=<optimized
> out>) at third_party/llvm/llvm/lib/Analysis/CallGraphSCCPass.cpp:442
>
> #10 (anonymous namespace)::CGPassManager::runOnModule
> (this=0x614000009840, M=...) at
> third_party/llvm/llvm/lib/Analysis/CallGraphSCCPass.cpp:498
>
> #11 0x000055555d39ca7a in (anonymous
> namespace)::MPPassManager::runOnModule (this=<optimized out>, M=...) at
> third_party/llvm/llvm/lib/IR/LegacyPassManager.cpp:1744
>
> #12 llvm::legacy::PassManagerImpl::run (this=0x619000020f50, M=...) at
> third_party/llvm/llvm/lib/IR/LegacyPassManager.cpp:1857
>
> #13 0x00005555588bfb58 in main (argc=<optimized out>, argv=<optimized
> out>) at third_party/llvm/llvm/tools/opt/opt.cpp:828
>
>
>
>
>
>
>
> I've spent the last 48 hours attempting to reduce a test case (both
> mechanically and manually). Unfortunately, I don't feel I have a useful
> reproducer to share... between creduce and bugpoint, I've gotten one target
> down to 68k of .bc (which disassembles to about 180k of .ll). (And it would
> take a lot of effort to redact before I can share it.)
>
>
>
> The stack trace pretty clearly points to an unchecked out-of-range return
> value here:
>
>
>
>
> https://git.llvm.org/klaus/llvm/blob/master/lib/Transforms/InstCombine/InstCombinePHI.cpp#L-1238
>
>
>
> I suspect your change is exposing a pre-existing bug; but unfortunately,
> it does very cleanly resolve to exactly r347934, and it occurs with
> multiple targets.
>
>
>
> Could you advise on a fix? Unfortunately, I'm not familiar with this code,
> so the best option I can offer is a revert. (If you won't able to author a
> timely fix, please let me know so I can submit the revert for you.)
>
>
>
> --David Jones
>
>
>
> On Thu, Nov 29, 2018 at 4:05 PM Warren Ristow via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> Author: wristow
> Date: Thu Nov 29 16:02:54 2018
> New Revision: 347934
>
> URL: http://llvm.org/viewvc/llvm-project?rev=347934&view=rev
> Log:
> [SCEV] Guard movement of insertion point for loop-invariants
>
> r320789 suppressed moving the insertion point of SCEV expressions with
> dev/rem operations to the loop header in non-loop-invariant situations.
> This, and similar, hoisting is also unsafe in the loop-invariant case,
> since there may be a guard against a zero denominator. This is an
> adjustment to the fix of r320789 to suppress the movement even in the
> loop-invariant case.
>
> This fixes PR30806.
>
> Differential Revision: https://reviews.llvm.org/D54713
>
> Added:
> llvm/trunk/test/Transforms/LoopVectorize/pr30806.ll
> Modified:
> llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp
>
> Modified: llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp?rev=347934&r1=347933&r2=347934&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp (original)
> +++ llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp Thu Nov 29
> 16:02:54 2018
> @@ -1732,49 +1732,50 @@ Value *SCEVExpander::expand(const SCEV *
> // Compute an insertion point for this SCEV object. Hoist the
> instructions
> // as far out in the loop nest as possible.
> Instruction *InsertPt = &*Builder.GetInsertPoint();
> - for (Loop *L = SE.LI.getLoopFor(Builder.GetInsertBlock());;
> - L = L->getParentLoop())
> - if (SE.isLoopInvariant(S, L)) {
> - if (!L) break;
> - if (BasicBlock *Preheader = L->getLoopPreheader())
> - InsertPt = Preheader->getTerminator();
> - else {
> - // LSR sets the insertion point for AddRec start/step values to
> the
> - // block start to simplify value reuse, even though it's an
> invalid
> - // position. SCEVExpander must correct for this in all cases.
> - InsertPt = &*L->getHeader()->getFirstInsertionPt();
> - }
> - } else {
> - // We can move insertion point only if there is no div or rem
> operations
> - // otherwise we are risky to move it over the check for zero
> denominator.
> - auto SafeToHoist = [](const SCEV *S) {
> - return !SCEVExprContains(S, [](const SCEV *S) {
> - if (const auto *D = dyn_cast<SCEVUDivExpr>(S)) {
> - if (const auto *SC =
> dyn_cast<SCEVConstant>(D->getRHS()))
> - // Division by non-zero constants can be hoisted.
> - return SC->getValue()->isZero();
> - // All other divisions should not be moved as they
> may be
> - // divisions by zero and should be kept within the
> - // conditions of the surrounding loops that guard
> their
> - // execution (see PR35406).
> - return true;
> - }
> - return false;
> - });
> - };
> - // If the SCEV is computable at this level, insert it into the
> header
> - // after the PHIs (and after any other instructions that we've
> inserted
> - // there) so that it is guaranteed to dominate any user inside the
> loop.
> - if (L && SE.hasComputableLoopEvolution(S, L) &&
> !PostIncLoops.count(L) &&
> - SafeToHoist(S))
> - InsertPt = &*L->getHeader()->getFirstInsertionPt();
> - while (InsertPt->getIterator() != Builder.GetInsertPoint() &&
> - (isInsertedInstruction(InsertPt) ||
> - isa<DbgInfoIntrinsic>(InsertPt))) {
> - InsertPt = &*std::next(InsertPt->getIterator());
> +
> + // We can move insertion point only if there is no div or rem operations
> + // otherwise we are risky to move it over the check for zero
> denominator.
> + auto SafeToHoist = [](const SCEV *S) {
> + return !SCEVExprContains(S, [](const SCEV *S) {
> + if (const auto *D = dyn_cast<SCEVUDivExpr>(S)) {
> + if (const auto *SC = dyn_cast<SCEVConstant>(D->getRHS()))
> + // Division by non-zero constants can be hoisted.
> + return SC->getValue()->isZero();
> + // All other divisions should not be moved as they may be
> + // divisions by zero and should be kept within the
> + // conditions of the surrounding loops that guard their
> + // execution (see PR35406).
> + return true;
> + }
> + return false;
> + });
> + };
> + if (SafeToHoist(S)) {
> + for (Loop *L = SE.LI.getLoopFor(Builder.GetInsertBlock());;
> + L = L->getParentLoop()) {
> + if (SE.isLoopInvariant(S, L)) {
> + if (!L) break;
> + if (BasicBlock *Preheader = L->getLoopPreheader())
> + InsertPt = Preheader->getTerminator();
> + else
> + // LSR sets the insertion point for AddRec start/step values to
> the
> + // block start to simplify value reuse, even though it's an
> invalid
> + // position. SCEVExpander must correct for this in all cases.
> + InsertPt = &*L->getHeader()->getFirstInsertionPt();
> + } else {
> + // If the SCEV is computable at this level, insert it into the
> header
> + // after the PHIs (and after any other instructions that we've
> inserted
> + // there) so that it is guaranteed to dominate any user inside
> the loop.
> + if (L && SE.hasComputableLoopEvolution(S, L) &&
> !PostIncLoops.count(L))
> + InsertPt = &*L->getHeader()->getFirstInsertionPt();
> + while (InsertPt->getIterator() != Builder.GetInsertPoint() &&
> + (isInsertedInstruction(InsertPt) ||
> + isa<DbgInfoIntrinsic>(InsertPt)))
> + InsertPt = &*std::next(InsertPt->getIterator());
> + break;
> }
> - break;
> }
> + }
>
> // Check to see if we already expanded this here.
> auto I = InsertedExpressions.find(std::make_pair(S, InsertPt));
>
> Added: llvm/trunk/test/Transforms/LoopVectorize/pr30806.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LoopVectorize/pr30806.ll?rev=347934&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/LoopVectorize/pr30806.ll (added)
> +++ llvm/trunk/test/Transforms/LoopVectorize/pr30806.ll Thu Nov 29
> 16:02:54 2018
> @@ -0,0 +1,65 @@
> +; RUN: opt -loop-vectorize -S < %s 2>&1 | FileCheck %s
> +
> +; Produced from test-case:
> +;
> +; void testGuardedInnerLoop(uint32_t *ptr, uint32_t denom, uint32_t
> numer, uint32_t outer_lim)
> +; {
> +; for(uint32_t outer_i = 0; outer_i < outer_lim; ++outer_i) {
> +; if (denom > 0) {
> +; const uint32_t lim = numer / denom;
> +;
> +; for (uint32_t i = 0; i < lim; ++i)
> +; ptr[i] = 1;
> +; }
> +; }
> +; }
> +
> +
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128-ni:1"
> +target triple = "x86_64-unknown-linux-gnu"
> +
> +define void @testGuardedInnerLoop(i32* %ptr, i32 %denom, i32 %numer, i32
> %outer_lim) {
> +entry:
> + %cmp1 = icmp eq i32 %outer_lim, 0
> + br i1 %cmp1, label %exit, label %loop1.preheader
> +
> +; Verify that a 'udiv' does not appear between the 'loop1.preheader'
> label, and
> +; whatever label comes next.
> +loop1.preheader:
> +; CHECK-LABEL: loop1.preheader:
> +; CHECK-NOT: udiv
> +; CHECK-LABEL: :
> + br label %loop1
> +
> +loop1:
> + %outer_i = phi i32 [ %inc1, %loop2.exit ], [ 0, %loop1.preheader ]
> + %0 = add i32 %denom, -1
> + %1 = icmp ult i32 %0, %numer
> + br i1 %1, label %loop2.preheader, label %loop2.exit
> +
> +; Verify that a 'udiv' does appear between the 'loop2.preheader' label,
> and
> +; whatever label comes next.
> +loop2.preheader:
> +; CHECK-LABEL: loop2.preheader:
> +; CHECK: udiv
> +; CHECK-LABEL: :
> + %lim = udiv i32 %numer, %denom
> + %2 = zext i32 %lim to i64
> + br label %loop2
> +
> +loop2:
> + %indvar.loop2 = phi i64 [ 0, %loop2.preheader ], [ %indvar.loop2.next,
> %loop2 ]
> + %arrayidx = getelementptr inbounds i32, i32* %ptr, i64 %indvar.loop2
> + store i32 1, i32* %arrayidx, align 4
> + %indvar.loop2.next = add nuw nsw i64 %indvar.loop2, 1
> + %cmp2 = icmp ult i64 %indvar.loop2.next, %2
> + br i1 %cmp2, label %loop2, label %loop2.exit
> +
> +loop2.exit:
> + %inc1 = add nuw i32 %outer_i, 1
> + %exitcond = icmp eq i32 %inc1, %outer_lim
> + br i1 %exitcond, label %exit, label %loop1
> +
> +exit:
> + ret void
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181205/26589184/attachment.html>
More information about the llvm-commits
mailing list