[PATCH] D67088: [PowerPC] extend PPCPreIncPrep Pass for ds/dq form

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

A few questions first.



================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:85
+static cl::opt<unsigned> MaxVarsPrep("ppc-formprep-max-vars",
                                  cl::Hidden, cl::init(16),
+  cl::desc("Potential common base number threshold per function for PPC loop "
----------------
We are also doing DS/DQ now, why we still keep the max-vars the same as before? Shall we increase it a little bit? 

Looks like NO according to original comments?
So, maybe we should still keep original comments about `which is a little over half of the allocatable register set`, so that it will be clearer about why we choose 16 here.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:90
+// Sum of following 3 per loop thresholds for all loops can not be larger
+// than MaxVarsPrep.
+// By default, we limit this to creating 9 PHIs for one loop.
----------------
Yes, 9 < 16, but why 9 here?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:91
+// than MaxVarsPrep.
+// By default, we limit this to creating 9 PHIs for one loop.
+static cl::opt<unsigned> MaxVarsUpdateForm("ppc-preinc-prep-max-vars",
----------------
Why we have 3/3/3 for each type ? Should they be treated equally? Do you have any statistics to support the decision here?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:139
+  // load/store with update like ldu/stdu, or Prefetch intrinsic.
+  enum InstrForm { UpdateForm = 1, DSForm = 4, DQForm = 16 };
+
----------------
Maybe add comment here about why we want to assign `4`, `16` in enum.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:170
 
+    /// Successful preparation number for Updata/DS/DQ form in all inner most
+    /// loops. One successful preparation will put one common base out of loop,
----------------
Typo: `Update` not `Updata`


================
Comment at: llvm/test/CodeGen/PowerPC/swaps-le-1.ll:167
 ; CHECK-P9-LABEL: @foo
-; CHECK-P9-DAG: lxvx
-; CHECK-P9-DAG: lxvx
-; CHECK-P9-DAG: lxvx
-; CHECK-P9-DAG: lxvx
-; CHECK-P9-DAG: lxvx
-; CHECK-P9-DAG: lxvx
-; CHECK-P9-DAG: lxvx
-; CHECK-P9-DAG: lxvx
-; CHECK-P9-DAG: lxvx
-; CHECK-P9-DAG: lxvx
-; CHECK-P9-DAG: lxvx
-; CHECK-P9-DAG: lxvx
+; CHECK-P9-DAG: lxv
+; CHECK-P9-DAG: lxv
----------------
This testcase was broken, see https://reviews.llvm.org/D69733 for a fix.
We may need to rebase after it.


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

https://reviews.llvm.org/D67088