[PATCH] D144226: [Loop-Interchange] Allow inner-loop only reductions

[Ankit via Phabricator via llvm-commits]

quic_aankit marked 6 inline comments as done.
quic_aankit added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:846
+    for (auto I = Begin; I != End; I++)
+      if ((isa<LoadInst>(I) || isa<StoreInst>(I)) &&
+          DI->depends(SI, &*I, false))
----------------
Meinersbur wrote:
> There are other instructions that can access memory, e.g. CallInst (as in `memset(...)`). `tightlyNested` might already check for such instructions; if so, please add a comment about this here.
You are right. TightlyNested will later check for presence of other instructions that may read or write to memory and we do not proceed with loop interchange in that case.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:884
+    // Load should only be used by the PHI node.
+    assert(Load->isSimple() && "Expecting a simple load!");
+    if (!Load->hasOneUser())
----------------
Meinersbur wrote:
> If I am not mistaken, there is no previous check whether all accesses are simple already. That is, it will crash ind debug builds if someone uses e.g. `std::atomic`. I think it should also `return False` in this case. 
At this time, `populateDependencyMatrix`  has already ensured that all the loads and stores are simple. 


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:945
+    // Insert a copy of Load in the Loop Header
+    Instruction *LI = Load->clone();
+    LI->setName(Load->getName() + ".demoted");
----------------
Meinersbur wrote:
> It seems unnecessary to clone the instruction and create a new one? Why not using the existing instruction and move it into the loop?
Not sure why I was cloning earlier. Fixed it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:957-959
+    Store->eraseFromParent();
+    LoopHeaderPHI->eraseFromParent();
+    Load->eraseFromParent();
----------------
Meinersbur wrote:
> Shouldn't the `LoopExitPHI` be removed as well?
Yep. It should have been if there are no users. Fixed it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1067
+          Instruction *UI = dyn_cast<Instruction>(U);
+          return !isRemovableInst(UI) && UI->getParent() &&
+                 (!PN || (!OuterInnerReductions.count(PN) &&
----------------
Meinersbur wrote:
> Unless you removed it yourself, shouldn't all instructions have a parent?
Seems like a mistake I made earlier. Fixed.


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

https://reviews.llvm.org/D144226