This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fix worklist management in DSE (PR44552)
ClosedPublic

Authored by nikic on Jan 15 2020, 1:28 PM.

Details

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=44552. We need to make sure that the store is reprocessed, because performing DSE may expose more DSE opportunities.

There is a slight caveat here though: We need to make sure that we add back the store the worklist first, because that means it will be processed after the operands of the removed store have been processed. This is a general bug in InstCombine worklist management that I hope to address at some point, but for now it means we need to do this manually rather than just returning the instruction as changed.

Diff Detail

Event Timeline

nikic created this revision.Jan 15 2020, 1:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2020, 1:28 PM
nikic marked an inline comment as done.Jan 15 2020, 1:32 PM

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.

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
1448

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'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?

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
1448

I agree it's probably better to return here. But that means we should remove the unnecessary loop increment: ++BBI.

nikic updated this revision to Diff 238362.Jan 15 2020, 1:55 PM
nikic marked an inline comment as done.

Remove unnecessary iterator increment. Reduce test case to use 100 instead of 1000 iterations.

spatel accepted this revision.Jan 15 2020, 2:01 PM

I would've taken the test down to 5 or 10 to make it easier to see without scrolling, but this is fine. LGTM.

This revision is now accepted and ready to land.Jan 15 2020, 2:01 PM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Jan 22 2020, 3:45 PM

Cherry-picked to 10.x in 7adf83beece381d153f8b4a67fa341d13f9c4ba2
Please let me know if there are any follow-ups.

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp