[PATCH] D134083: [IVDescriptors] Before moving an instruction in SinkAfter checking if it is target of other instructions

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a subscriber: aeubanks.
fhahn added a comment.

Thanks for the patch! This should fix an issue also mentioned at  D119661 <https://reviews.llvm.org/D119661>



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1037
+          });
+      if (SuccIt != std::end(SinkAfter))
+        return false;
----------------
Simpler to use `any_of`?, as in
```
      if (any_of(SinkAfter, [SinkCandidate](const std::pair<Instruction *, Instruction *> &P) {
            return P.second == SinkCandidate;
                 }))

```


================
Comment at: llvm/test/Transforms/LoopVectorize/sinkafter.ll:3
+
+; Make sure LLVM doesn't generate wrong data in SinkAfter, and causes crash in
+; loop vectorizer.
----------------
The existing tests for higher-order recurrences are in llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains.ll. Could you move your test there as well?

To improve readability, the IR tests there try to use descriptive names, like `%for.1` for recurrence phis, `%iv` for indications and so on. It would be good to try to update the names here to make the test easier to read, rather than using auto-generated names.

Also, @aeubanks posted a simpler test case in the comments for D119661, which may be good to use.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134083/new/

https://reviews.llvm.org/D134083