This is an archive of the discontinued LLVM Phabricator instance.

[NFCI][LoopUnrollAndJam] Minor changes.
ClosedPublic

Authored by Whitney on Jan 22 2020, 8:22 AM.

Details

Summary
  1. Add assertions.
  2. Use SmallVector instead of std::vector
  3. Verify more analyses.

These changes are moved out of https://reviews.llvm.org/D73129 to simplify that review.

Diff Detail

Event Timeline

Whitney created this revision.Jan 22 2020, 8:22 AM
fhahn added a subscriber: fhahn.Jan 22 2020, 8:38 AM
fhahn added inline comments.
llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp
536

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.

Whitney updated this revision to Diff 239614.Jan 22 2020, 8:43 AM

Addressed Dave's comment.

Whitney marked an inline comment as done.Jan 22 2020, 8:54 AM
Whitney added inline comments.
llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp
536

ok, I will revert my change here. The main reason I did that is it is easier to understand.

Whitney updated this revision to Diff 239624.Jan 22 2020, 9:23 AM
Whitney edited the summary of this revision. (Show Details)

Addressed Florian's comment.

Whitney marked an inline comment as done.Jan 22 2020, 9:24 AM

Looks like some nice cleanups.

llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp
384–385

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.

Whitney marked an inline comment as done.Jan 22 2020, 4:52 PM
Whitney added inline comments.
llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp
384–385

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?

dmgreen added inline comments.Jan 23 2020, 1:16 AM
llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp
384–385

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.

Whitney updated this revision to Diff 239926.Jan 23 2020, 9:41 AM
Whitney marked 3 inline comments as done.
Whitney added inline comments.
llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp
384–385

Created https://reviews.llvm.org/D73277 to address this.

dmgreen accepted this revision.Jan 24 2020, 10:20 AM

Nice one. What's left here looks good to me.

This revision is now accepted and ready to land.Jan 24 2020, 10:20 AM
This revision was automatically updated to reflect the committed changes.