[PATCH] D29425: [SLP] Use SCEV to sort memory accesses

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added inline comments.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1081
+    if (CurrObj != Obj0) {
+      Sorted.append(VL.begin(), VL.end());
+      return;
----------------
mkuper wrote:
> mssimpso wrote:
> > Hi Michael,
> > 
> > I must have missed this the first time around, but this looks a little confusing to me. If the accesses aren't sortable, why are we adding them to Sorted? I would think the caller would need some signal that sorting was unsuccessful - either an empty Sorted or a returned bool. Am I missing something obvious?
> No, you're not missing anything.
> 
> It's just that the API currently doesn't allow for the possibility of failure, so the callers blindly use the "sorted" array. This made sense for the previous version because it did not explicitly fail (it would just produce a meaningless order in cases where things weren't really sortable), and I didn't notice this when reviewing the original patch.
> I was planning to fix the API up in a follow-up patch, but if you want to see it in the review, I can update it with that.
A follow-on is perfectly fine - I just wanted to make sure I wasn't confusing myself. Thanks for the clarification!


https://reviews.llvm.org/D29425