[PATCH] D73204: [NFCI][LoopUnrollAndJam] Minor changes.

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:384
       for (Instruction &I : *NewBlock) {
-        ::remapInstruction(&I, LastValueMap);
         if (auto *II = dyn_cast<IntrinsicInst>(&I))
----------------
Whitney wrote:
> dmgreen wrote:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73204