<div dir="ltr">Yes, it would be very good to have a test-case.<div>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.<div><br></div><div>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. :-)</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 7, 2017 at 10:45 PM, Alexey Bataev <span dir="ltr"><<a href="mailto:a.bataev@hotmail.com" target="_blank">a.bataev@hotmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Michael,<br>
<br>
1. It is quite hard to add a test case because we have a random crash.<br>
Reproducer from the PR is not crashed on my side anymore while still<br>
causes crashes for others. Should I add this test case?<br>
<br>
2. SCEV is used during all of `vectorizeGEPIndices()`. But we can break<br>
SCEV during vectorizing of BasicBlock that is part of a loop.<br>
ScalarEvolution code has a cache of SCEV nodes that are not used<br>
anymore, but loop dispositions for these nodes are still active. This<br>
SCEV nodes may be reused later, for example, during some loop invariants<br>
folding. If folding is successful, we're asking for a new SCEV node for<br>
the fold result and get a SCEV node from cache that has its disposition<br>
`loop variant` rather than `loop invariant`. After that all SCEV<br>
operations may treat this SCEV node in the wrong way and it may crash<br>
the compiler (as it happens in this PR, when the following SCEV<br>
operations expect all SCEVs to be `loop invariant` while some of them<br>
are `loop variant` because of the wrong disposition of the cached SCEV<br>
node). To solve it we need to clear the info about loop dispositions of<br>
SCEV nodes to make ScalarEvolution to recalculate them. This is what is<br>
done by the ScalarEvolution code if some instructions are removed.<br>
<br>
I talked with Sanjoy. He said that this may happen because of some<br>
LICM-like optimizations. SLP vectorizer may move code inside of the<br>
basic blocks too (instruction scheduler tries to bundle all vectorized<br>
instruction into a bunch, we're creating new instructions) and thus some<br>
of SCEV nodes are incorrectly invalidated.<br>
<br>
-------------<br>
Best regards,<br>
Alexey Bataev<br>
<br>
08.02.2017 0:24, Michael Kuperstein via Phabricator пишет:<br>
<div class="HOEnZb"><div class="h5">> mkuper added a comment.<br>
><br>
> Test case?<br>
><br>
> Also, this looks a bit weird. Could you explain what exactly is going wrong?<br>
> 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.<br>
><br>
><br>
> <a href="https://reviews.llvm.org/D29641" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D29641</a><br>
><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div>