[PATCH] D67088: [PowerPC] extend PPCPreIncPrep Pass for ds/dq form
Jinsong Ji via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 5 13:52:25 PST 2019
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
More information about the llvm-commits
mailing list