[PATCH] Fix loop rerolling pass failure with non-consant loop lower

[Hal Finkel]

----- Original Message -----
> From: "David Peixotto" <dpeixott at codeaurora.org>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Friday, January 3, 2014 11:25:32 AM
> Subject: RE: [PATCH] Fix loop rerolling pass failure with non-consant loop lower
> 
> Hal, thanks for the review. I made all your suggested changes and
> committed in r198425.

Great! Can you open a new bug report about the fact that the loop is not rerolled on 64-bit targets (is this all 64-bit targets, or just 64-bit variants of a particular target?)

Thanks again,
Hal

> 
> > -----Original Message-----
> > From: Hal Finkel [mailto:hfinkel at anl.gov]
> > Sent: Wednesday, January 01, 2014 4:39 PM
> > To: David Peixotto
> > Cc: llvm-commits at cs.uiuc.edu
> > Subject: Re: [PATCH] Fix loop rerolling pass failure with
> > non-consant loop
> > lower
> > 
> > David,
> > 
> > Thanks for working on this! This looks good; a few minor comments:
> > 
> > +        // Iteration count scev minus 1
> > 
> > Please capitalize SCEV.
> > 
> > +        const SCEV *ICSCEVm1 = SE->getMinusSCEV(ICSCEV,
> > + SE->getConstant(ICSCEV->getType(), 1));
> > 
> > This line is too long. Also, please name this variable ICMinus1SCEV
> > for
> > consistency with the other variables.
> > 
> > +        Value *ICm1; // Iteration count minus 1
> > 
> > Please name this ICMinus1.
> > 
> > +
> > +
> > +attributes #0 = { nounwind "less-precise-fpmad"="false"
> > +"no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"
> > +"no-infs-fp-math"="false" "no-nans-fp-math"="false"
> > +"stack-protector-buffer-size"="8" "unsafe-fp-math"="false"
> > +"use-soft-float"="true" }
> > +
> > +!llvm.ident = !{!0}
> > +
> > +!0 = metadata !{metadata !"Clang 3.1"}
> > +!1 = metadata !{metadata !2, metadata !2, i64 0}
> > +!2 = metadata !{metadata !"int", metadata !3, i64 0}
> > +!3 = metadata !{metadata !"omnipotent char", metadata !4, i64 0}
> > +!4 = metadata !{metadata !"Simple C/C++ TBAA"}
> > +!5 = metadata !{metadata !6, metadata !6, i64 0}
> > +!6 = metadata !{metadata !"float", metadata !3, i64 0}
> > 
> > Please remove any unnecessary attributes and metadata.
> > 
> > With these changes, LGTM.
> > 
> >  -Hal
> > 
> > ----- Original Message -----
> > > From: "David Peixotto" <dpeixott at codeaurora.org>
> > > To: llvm-commits at cs.uiuc.edu
> > > Cc: "Hal Finkel" <hfinkel at anl.gov>
> > > Sent: Friday, December 20, 2013 7:09:41 PM
> > > Subject: [PATCH] Fix loop rerolling pass failure with non-consant
> > > loop
> > > lower
> > >
> > > Proposed fix for PR18290:
> > > http://llvm.org/bugs/show_bug.cgi?id=18290
> > >
> > > The loop rerolling pass was failing with an assertion failure
> > > from a
> > > failed cast on loops like this:
> > >
> > >   void foo(int *A, int *B, int m, int n) {
> > >     for (int i = m; i < n; i+=4) {
> > >       A[i+0] = B[i+0] * 4;
> > >       A[i+1] = B[i+1] * 4;
> > >       A[i+2] = B[i+2] * 4;
> > >       A[i+3] = B[i+3] * 4;
> > >     }
> > >   }
> > >
> > > The code was casting the SCEV-expanded code for the new induction
> > > variable to a phi-node. When the loop had a non-constant lower
> > > bound,
> > > the SCEV expander would end the code expansion with an add insted
> > > of a
> > > phi node and the cast would fail.
> > >
> > > It looks like the cast to a phi node was only needed to get the
> > > induction variable value coming from the backedge to compute the
> > > end
> > > of loop condition. This patch changes the loop reroller to
> > > compare the
> > > induction variable to the number of times the backedge is taken
> > > instead of the iteration count of the loop. In other words, we
> > > stop
> > > the loop when the current value of the induction variable ==
> > > IterationCount-1. Previously, the comparison was comparing the
> > > induction variable value from the next iteration ==
> > > IterationCount.
> > >
> > > This problem only seems to occur on 32-bit targets. For some
> > > reason,
> > > the loop is not rerolled on 64-bit targets.
> > >
> > 
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory