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

[Jinsong Ji via Phabricator via llvm-commits]

jsji requested changes to this revision.
jsji added a comment.
This revision now requires changes to proceed.

Overall looks promising!



================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:105
+
+// If would be not profitable if one common base has only one load/store, ISEL
+// can choose best load/store form based on offset for single load/store.
----------------
`If would be not profitable if one common base` -> `It would not be profitable if the common base`?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:106
+// If would be not profitable if one common base has only one load/store, ISEL
+// can choose best load/store form based on offset for single load/store.
+// Set default to minmal profitable value 2 and make it as a option.
----------------
`can choose` -> `should already be able to `


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:107
+// can choose best load/store form based on offset for single load/store.
+// Set default to minmal profitable value 2 and make it as a option.
+static cl::opt<unsigned> DispFormPrepMinThreshold("ppc-dispprep-min-threshold",
----------------
`Set default to minmal profitable value 2 and make it as a option.` -> `Set minimal profitable value default to 2 and make it as an option`.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:207
     /// update all other elements in \p BucketChain accordingly.
+    /// \p DispConstraint is used to find the best base element.
+    /// If success, best base element must be stored as the first element of
----------------
There is no parameter of `DispConstraint`. Describe `InstrFrom` instead.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:364
+                                                    InstrForm Form) {
+  // ReminderOffsetInfo details:
+  // key:            value of (Offset urem DispConstraint). For DSForm, it can
----------------
`ReminderOffsetInfo` -> `RemainderOffsetInfo` for all places below.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:368
+  // first of pair:  first BucketElement index with a reminder key. for key 0,
+  //                 this value must be 0.
+  // second of pair: number of load/stores with the same reminder.
----------------
`first BucketElement index with a reminder key...` -> `the index of first BucketElement whose remainder is equal to key. `


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:386
+  // number of load/store with same reminder.
+  // TODO: adjust the base select strategy according to load/store offset
+  // distribution.
----------------
Maybe use `FIXME` instead of `TODO` here to be consistent.
`base select strategy`-> `base selection strategy`.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:412
+  if (MaxCountReminder == 0)
+    return true;
+
----------------
Why we return true when no update needed? We don't need to rewrite anything.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:507
+    return MadeChange;
+  // For some DS form load/store instructions, it can also be a update form,
+  // if the increasing step is a multipler of 4.
----------------
`also be a update form` -> `also be an update form`


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:508
+  // For some DS form load/store instructions, it can also be a update form,
+  // if the increasing step is a multipler of 4.
+  bool CanPreInc =
----------------
`if the increasing step is a multipler of 4` -> `if the stride is a multiple of 4` ?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:511
+      (Form == UpdateForm ||
+       ((Form == DSForm) && !BasePtrIncSCEV->getAPInt().urem(4)));
+  if (CanPreInc)
----------------
Why we would like to always prefer `UpdateForm`?
What if I would like to disable `UpdateForm` and only do DS/DQ form ?

Also even if we prefer `UpdateForm`, do we need to consider the `MaxVarsUpdateForm` here? Or else, we may actually generate more update form than expected? 


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

https://reviews.llvm.org/D67088