- Add assertions.
- Use SmallVector instead of std::vector
- Verify more analyses.
These changes are moved out of https://reviews.llvm.org/D73129 to simplify that review.
Could you provide context on why not using DTU is preferred here? Is it because the existing copy/cloning code in the pass changes the DT in place? I just had a quick look and it looks like the DT is not accessed in between cloning until simplifying the loops after unrolling.
DTU has the advantage that we can preserve the PDT also for almost no extra cost (just pass a PDT to the DTU additionally). Also, batch updates can be much faster, given that the update set is large enough.
I think unless there's a strong reason, we should try to replace the existing uses of in-place updates with DTU update.
Looks like some nice cleanups.
Why does the unroller have it's own version of this function, and how is it different to RemapInstruction?
If this is no longer needs to be shared with the unroller, it can be removed from UnrollLoop.h and made static in LoopUnroll.cpp.
Looks to me that RemapInstruction is a superset of ::remapInstruction. I tried locally changing remapInstruction to call RemapInstruction(I, VMap, RF_NoModuleLevelChanges | RF_IgnoreMissingLocals); and all lit tests passed. Should be safe to remove ::remapInstruction. Should I do that in this patch?
There is a possibility that the loop unrollers remapInstruction is a subset for (compiler time) performance reasons. It may know that it needs to do less, so only handles what is required.
But there is a chance that it's just very old and no-one has noticed they are the same in the past though. Maybe look through git history to see if anything looks obvious?
If it does look OK to remove, we should probably change the Unroller and UnrollAndJam in the same patch. I imagine if there is a problem, the same conditions would apply to both.