[PATCH] D69538: [ModuloSchedule] Fix experimental modulo expansion for data loop carried dependencies.

[James Molloy via Phabricator via llvm-commits]

jmolloy added a comment.

Thanks Thomas!



================
Comment at: llvm/include/llvm/CodeGen/ModuloSchedule.h:303
 
+  SmallVector<MachineInstr *, 4> IllegalPhisToDelete;
+
----------------
Please add a docstring


================
Comment at: llvm/include/llvm/CodeGen/ModuloSchedule.h:328
+  // block.
+  void FilterInstructions(MachineBasicBlock *MB, int MinStage);
+  // Move instructions of the given stage from sourceBB to DestBB. Remap the phi
----------------
Please follow the style of the file: filterInstructions().


================
Comment at: llvm/include/llvm/CodeGen/ModuloSchedule.h:331
+  // instructions to keep a valid IR.
+  void MoveStageInBB(MachineBasicBlock *DestBB, MachineBasicBlock *SourceBB,
+                     int Stage);
----------------
same here. "moveStageInBB" is a little ambiguous; can I suggest something like "moveStageBetweenBlocks"?


================
Comment at: llvm/lib/CodeGen/ModuloSchedule.cpp:10
 #include "llvm/CodeGen/ModuloSchedule.h"
+#include "third_party/llvm/llvm-project/llvm/include/llvm/CodeGen/Register.h"
 #include "llvm/ADT/StringExtras.h"
----------------
incorrect include here. Please make sure this builds.


================
Comment at: llvm/lib/CodeGen/ModuloSchedule.cpp:1659
+  }
+  for (auto *P : PhiToDelete) {
+    P->eraseFromParent();
----------------
nit: remove brackets


================
Comment at: llvm/lib/CodeGen/ModuloSchedule.cpp:1709
+  // We first peel number of stages minus one epilogue. Then we remove dead
+  // stages and reorder instructions based on there stage. If we have 3 stages
+  // we generate first:
----------------
s/there/their/


================
Comment at: llvm/test/CodeGen/Hexagon/swp-conv3x3-nested.ll:2
 ; RUN: llc -march=hexagon < %s -pipeliner-experimental-cg=true | FileCheck %s
-; XFAIL: *
-; LSR changes required.
----------------
Was this xfail deliberately removed?


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

https://reviews.llvm.org/D69538