[PATCH] D29641: [SLP] Fix for PR31847: Assertion failed: (isLoopInvariant(Operands[i], L) && "SCEVAddRecExpr operand is not loop-invariant!")

[Michael Kuperstein via llvm-commits]

Yes, it would be very good to have a test-case.
Especially if you haven't been able to reproduce the crash on your side -
this makes me even less convinced this is the right fix. As I wrote above,
it looks like the change uses a fairly big hammer, and I don't know whether
"after every change" is the right place to apply said hammer. So it'd be
great to understand what exactly breaks here and why.

That said, Sanjoy, if you think this is the right thing to do here, feel
free to approve it, I defer to your judgement on when to invalidate parts
of SCEV. :-)

On Tue, Feb 7, 2017 at 10:45 PM, Alexey Bataev <a.bataev at hotmail.com> wrote:

> Michael,
>
> 1. It is quite hard to add a test case because we have a random crash.
> Reproducer from the PR is not crashed on my side anymore while still
> causes crashes for others. Should I add this test case?
>
> 2. SCEV is used during all of `vectorizeGEPIndices()`. But we can break
> SCEV during vectorizing of BasicBlock that is part of a loop.
> ScalarEvolution code has a cache of SCEV nodes that are not used
> anymore, but loop dispositions for these nodes are still active. This
> SCEV nodes may be reused later, for example, during some loop invariants
> folding. If folding is successful, we're asking for a new SCEV node for
> the fold result and get a SCEV node from cache that has its disposition
> `loop variant` rather than `loop invariant`. After that all SCEV
> operations may treat this SCEV node in the wrong way and it may crash
> the compiler (as it happens in this PR, when the following SCEV
> operations expect all SCEVs to be `loop invariant` while some of them
> are `loop variant` because of the wrong disposition of the cached SCEV
> node). To solve it we need to clear the info about loop dispositions of
> SCEV nodes to make ScalarEvolution to recalculate them. This is what is
> done by the ScalarEvolution code if some instructions are removed.
>
> I talked with Sanjoy. He said that this may happen because of some
> LICM-like optimizations. SLP vectorizer may move code inside of the
> basic blocks too (instruction scheduler tries to bundle all vectorized
> instruction into a bunch, we're creating new instructions) and thus some
> of SCEV nodes are incorrectly invalidated.
>
> -------------
> Best regards,
> Alexey Bataev
>
> 08.02.2017 0:24, Michael Kuperstein via Phabricator пишет:
> > mkuper added a comment.
> >
> > Test case?
> >
> > Also, this looks a bit weird. Could you explain what exactly is going
> wrong?
> > I'm asking because this somehow seems like it's the wrong granularity
> for this - after any change, you invalidate the disposition of the
> innermost loop that contains the basic block in which the root/seed lives.
> I'm not sure why this is the right thing to do.
> >
> >
> > https://reviews.llvm.org/D29641
> >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170208/fd57adf2/attachment.html>