[PATCH] D27216: [SCEVExpand] do not hoist divisions by zero (PR30935)

[Sanjoy Das via llvm-commits]

Hi Andy,

The problem here is that SCEVExpander actively tries to _hoist_
expansions out of a loop.  This can be a problem if the hoisting ends
up speculating a division beyond a point where it is safe (e.g. by
hoisting a div by zero out of a dynamically dead branch).

The obvious fix is to prevent SCEVExpander from hoisting unsafe
operations like division.  That is what this patch does, and is what I
did in an earlier patch.  However that approach fails as is because
there seems to be at least one case where clients of SCEVExpander
_rely_ on SCEVExpander to hoist certain subexpressions (including
unsafe ones):  the start and step values of add recurrences.  If a
SCEVExpander client wants to expand an add recurrence in the middle of
the loop, SCEVExpander uses the *same* hoisting-as-optimization logic
to make sure that despite the insertion point being asked for is in
the middle of the loop, the start and step values are expanded into
the preheader.

-- Sanjoy

On Wed, Nov 30, 2016 at 10:01 AM, Andrew Trick <atrick at apple.com> wrote:
>
> On Nov 30, 2016, at 1:16 AM, Sanjoy Das via Phabricator
> <reviews at reviews.llvm.org> wrote:
>
> sanjoy added a comment.
>
> Hi,
>
> I don't think this patch properly addresses PR30976 -- it only hides the
> symptom.  For instance, this new test case crashes SCEVExpander even with
> this fix: https://reviews.llvm.org/differential/diff/79708/
>
> I think the fundamental problem is that there is no clear distinction in
> SCEVExpander between hoisting for optimization and hoisting for correctness
> (i.e. for the right domination properties to hold). We need to fix that
> before we can fix PR30976.
>
>
> I didn’t look closely at this, but in general SCEVExpander doesn’t have the
> information to determine whether an expansion would be legal (let alone
> profitable). That all needs to be determined before ever calling
> SCEVExpander. If you need to limit code motion within an expression, then
> you need to break up the expression and call SCEVExpander separately on each
> subexpression.
>
>
> -Andy



-- 
Sanjoy Das
http://playingwithpointers.com