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

[Whitney Tsang via Phabricator via llvm-commits]

Whitney 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))
----------------
dmgreen wrote:
> 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.
Created https://reviews.llvm.org/D73277 to address this.


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

https://reviews.llvm.org/D73204