HomePhabricator

Do actual DCE in LoopUnroll (try 3)

Authored by reames on May 17 2021, 2:10 PM.

Description

Do actual DCE in LoopUnroll (try 3)

Recommitting after fixing a bug found post commit. Amusingly, try 1 had been correct, and by reverting to incorporate last minute review feedback, I introduce the bug. Oops. :)

The problem was that recursively deleting an instruction can delete instructions beyond the current iterator (via a dead phi), thus invalidating iteration. Test case added in LoopUnroll/dce.ll to cover this case.

LoopUnroll does a limited DCE pass after unrolling, but if you have a chain of dead instructions, it only deletes the last one. Improve the code to recursively delete all trivially dead instructions.

Differential Revision: https://reviews.llvm.org/D102511

Details

Event Timeline

Herald added a subscriber: Restricted Project. ยท View Herald TranscriptMay 17 2021, 2:50 PM

This is causing an assert in chromium builds; error message looks like this:

clang++: /b/s/w/ir/cache/builder/src/third_party/llvm/llvm/lib/Transforms/Utils/Local.cpp:540: void llvm::RecursivelyDeleteTriviallyDeadInstructions(llvm::SmallVectorImpl<llvm::WeakTrackingVH>&, const llvm::TargetLibraryInfo*, llvm::MemorySSAUpdater*, std::function<void(llvm::Value*)>): Assertion `isInstructionTriviallyDead(I, TLI) && "Live instruction found in dead worklist!"' failed.

I'm currently making a reduced repro -

attached a repro; sorry it's a bit long and not extremely readable

repros with clang -emit-llvm -O2 t.cc

reverted for now

reverted for now

Thank you for the revert. JFYI, I was not able to extract an IR test case for this. If you can, I'd appreciate it.

The issue turned out to be a latent bug in simplifyLoopIVs. Some of the "DeadInsts" aren't actually dead. Instead of tracking down that issue and fixing it, I'm simply going to recommit without that (intended to be NFC) portion of the change.

Resubmitted with a fix (449d14e), please revert if you see further problems.