[PATCH] D72807: [InstCombine] Fix worklist management in DSE (PR44552)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D72807#1822794 <https://reviews.llvm.org/D72807#1822794>, @nikic wrote:

> I've added the complete original test case with `-instcombine-infinite-loop-threshold=2`. It would also be possible to reduce it (it just wouldn't need the full 1000 iterations that trigger the default threshold), but as the test now runs quickly even on a debug build, I figured keeping the original is fine.


I think it would be better to shrink the test based on size if not speed. The current 1000 limit is arbitrary, and we can control when we hit the assert with the threshold flag, right?



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1448
         eraseInstFromFunction(*PrevSI);
-        continue;
+        return nullptr;
       }
----------------
nikic wrote:
> Technically this could stay `continue`, but I'm uncomfortable with the fact that this just falls through to potentially doing other transforms below. I think it's better to DCE things here and let the instruction be reprocessed.
I agree it's probably better to return here. But that means we should remove the unnecessary loop increment: ++BBI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72807