[llvm] r222093 - ScalarEvolution: HowFarToZero was wrongly using signed division

[Sebastian Pop]

On Sun, Nov 16, 2014 at 4:19 AM, Tobias Grosser <tobias at grosser.es> wrote:
> On 16.11.2014 08:30, David Majnemer wrote:
>>
>> Author: majnemer
>> Date: Sun Nov 16 01:30:35 2014
>> New Revision: 222093
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=222093&view=rev
>> Log:
>> ScalarEvolution: HowFarToZero was wrongly using signed division
>>
>> HowFarToZero was supposed to use unsigned division in order to calculate
>> the backedge taken count.  However, SCEVDivision::divide performs signed
>> division.  Unless I am mistaken, no users of SCEVDivision actually want
>> signed arithmetic: switch to udiv and urem.

findArrayDimensionsRec requires signed division in case Step is negative:

    // Normalize the terms before the next call to findArrayDimensionsRec.
    const SCEV *Q, *R;
    SCEVDivision::divide(SE, Term, Step, &Q, &R);

>>
>> This fixes PR21578.
>
>
> This commit breaks a delinearization test case in polly.
>

Tobi, could you say which testcase broke?

> I copied Sebastian who originally wrote the divide operation exactly as part
> of our delinearization analysis. He is possibly the best person to comment
> on this.
>

Most likely we will need to duplicate the code of SCEVDivision into a
SCEVUDivide.
The code that reused SCEVDivide contains this comment that expects SCEVDivision
to perform an unsigned division:

+  // If the step exactly divides the distance then unsigned divide computes the
+  // backedge count.
+  const SCEV *Q, *R;
+  ScalarEvolution &SE = *const_cast<ScalarEvolution *>(this);
+  SCEVDivision::divide(SE, Distance, Step, &Q, &R);

commit ed05e3703e07dfeae33acdd3cc5ad07b5f5b86c6
Author: Mark Heffernan <meheff at google.com>
Date:   Fri Oct 10 17:39:11 2014 +0000

    This patch de-pessimizes the calculation of loop trip counts in
    ScalarEvolution in the presence of multiple exits. Previously all
    loops exits had to have identical counts for a loop trip count to be
    considered computable. This pessimization was implemented by calling
    getBackedgeTakenCount(L) rather than getExitCount(L, ExitingBlock)
    inside of ScalarEvolution::getSmallConstantTripCount() (see the FIXME
    in the comments of that function). The pessimization was added to fix
    a corner case involving undefined behavior (pr/16130). This patch more
    precisely handles the undefined behavior case allowing the pessimization
    to be removed.

    ControlsExit replaces IsSubExpr to more precisely track the case where
    undefined behavior is expected to occur. Because undefined behavior is
    tracked more precisely we can remove MustExit from ExitLimit. MustExit
    was used to track the case where the limit was computed potentially
    assuming undefined behavior even if undefined behavior didn't necessarily
    occur.


    git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@219517
91177308-0d34-0410-b5e6-96231b3b80d8