[PATCH] D55642: [X86] Fix assert fails in pass X86AvoidSFBPass

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM, with a small nit.

In general, I'm not sure whether the overall behavior here makes sense. Especially in the case where the blocking stores don't come from different BBs, it would make more sense to me to preserve the last (in program order) of the overlapping stores, rather than the smallest, as that's the store that will be forwarded. If the stores come from multiple predecessors (like here) it's less clear which choice is right.

But in any case, that would require larger changes and shouldn't block this assertion fix.



================
Comment at: lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:644
 
-  int64_t PrevDisp = BlockingStoresDispSizeMap.begin()->first;
-  unsigned PrevSize = BlockingStoresDispSizeMap.begin()->second;
-  SmallVector<int64_t, 2> ForRemoval;
-  for (auto DispSizePair = std::next(BlockingStoresDispSizeMap.begin());
-       DispSizePair != BlockingStoresDispSizeMap.end(); ++DispSizePair) {
-    int64_t CurrDisp = DispSizePair->first;
-    unsigned CurrSize = DispSizePair->second;
-    if (CurrDisp + CurrSize <= PrevDisp + PrevSize) {
-      ForRemoval.push_back(PrevDisp);
+  SmallVector<std::pair<const int64_t, unsigned>, 0> DispSizeStack;
+  for (auto DispSizePair : BlockingStoresDispSizeMap) {
----------------
I don't think the `const` on a non-pointer here is useful.


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

https://reviews.llvm.org/D55642