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

[Allen zhong via Phabricator via llvm-commits]

Allen marked an inline comment as done.
Allen added a comment.

use return Previous->comesBefore(LastPrev)



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:932
+        return true;
+    }
 
----------------
fhahn wrote:
> 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.
thanks for your comment very much!


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

https://reviews.llvm.org/D118558