[PATCH] D118558: [IVDescriptors] Support FOR where we have multiple sink pointed

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:932
+        return true;
+    }
 
----------------
Allen wrote:
> fhahn wrote:
> > I think this handles the case where Previous doesn't come before LastPrev incorrectly. I added an additional test that crashes with the current version of the patch (02ee3fbff816).
> > 
> > To start with, you should be able to use `return Previous->comesBefore(LastPrev);` here. The reasoning is that if Previous comes before LastPrev, `SinkCandidate` already gets sunk after `Previous`, so there's nothing to do here.
> > 
> > I put up a follow-up patch (D118642), that should handle the other case correctly.
> Yes, Thanks a lot for extending.
> 
> what need I do next step ? do you mind squash your patch D118642, or just fix the crash of your new added case 02ee3fbff816 ?
> what need I do next step ? do you mind squash your patch D118642, or just fix the crash of your new added case 02ee3fbff816 ?

As suggested in my comment, you should be able to update the patch to use

`return Previous->comesBefore(LastPrev)`

instead of 
```
      if (Previous->comesBefore(LastPrev))
        return true;
```

to fix the issue.


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

https://reviews.llvm.org/D118558