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

[ChenZheng via Phabricator via llvm-commits]

shchenz marked 19 inline comments as done.
shchenz added a comment.

Very appriciate for your reviewing.



================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:412
+  if (MaxCountReminder == 0)
+    return true;
+
----------------
jsji wrote:
> Why we return true when no update needed? We don't need to rewrite anything.
Sorry for confusion comment. We still need to rewrite the load/stores based on the first value. Here I mean we don't need to adjust the elements in BucketChain, since first value is most profitable and all elements already substract first value when collecting them.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:511
+      (Form == UpdateForm ||
+       ((Form == DSForm) && !BasePtrIncSCEV->getAPInt().urem(4)));
+  if (CanPreInc)
----------------
jsji wrote:
> 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? 
This is a good question. Yes, for my understanding, I always try to use update form since we can save one addi, no explicit addi to loop iteration variable. But as you said, it will replace DS Form with Update Form. Two issues we meet:
issue1: counter DSFormChainRewritten/UpdateFormChainRewritten are not accurate. 
issue2: Update form number is not under control of `MaxVarsUpdateForm` as `MaxVarsUpdateForm` is only used when collect update candidates.

issue1 is fixed.
Add an option `PreferUpdateForm` to control should we choose update form if one chain is both ds and update form. 
If this option is false, no issue for now. All three forms have no interact.
If this option is true, we still meet issue2. The original reason we add `MaxVarsUpdateForm` and `MaxVarsDSForm` is to prevent from inserting too many PHIs and since we already use this limit in collecting candidates, so I guess no matter we choose update or ds, no impact to the total PHI numbers, One more PHI in update form, but one less PHI in ds form? 
Default value for `PreferUpdateForm` is true.


================
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
----------------
jsji wrote:
> This testcase was broken, see https://reviews.llvm.org/D69733 for a fix.
> We may need to rebase after it.
Yeah, Thanks for reminder, I will rebase after that patch gets committed.


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

https://reviews.llvm.org/D67088