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

[Andrew Trick via llvm-commits]

> On Dec 7, 2016, at 12:02 PM, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:
> 
> 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

Thanks for explaining. I would say the the SCEV expression has already hoisted the start and step values by forming an AddRec of that form. I’m not sure how the SCEVExpander would/could know that the loop invariant expressions were “hoisted”. In the past, I’ve worked around this problem by checking whether the SCEV expression is safely materializable before calling the expander. I’m not actually sure why the same problem is cropping up again.

-Andy

> 
> 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