[PATCH] D108750: [PowerPC] common chains to reuse offsets to reduce register pressure

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

Thanks for your review. @jsji Updated accordingly.



================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:498
+
+  for (const auto &BB : L->blocks())
+    for (auto &J : *BB) {
----------------
jsji wrote:
> We already collected both Dform and DQform buckets before, can we reuse some info to do early exit?
Good idea. We can share the info among all forms. I split this into a new patch D112196.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:541
+  // No reuseable offset, skip this bucket.
+  if (TotalCount == 1)
+    return false;
----------------
jsji wrote:
> How about 2 reusable offset in 20 offsets? Maybe we should introduce a heuristic to control whether we want to skip the bucket? eg: if the ratio of reusable is very low?
I added a new threshold `MaxVarsChainCommon` in the new patch which will prevent from commoning too many chains.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:545
+  // All elements are increased by Offset.
+  // The number of new bases should both be sqrt(EleNum).
+  if (!SawSeparater) {
----------------
jsji wrote:
> Why `sqrt(EleNum)`? 
The minimal for `(EleNum/x + x)` (1 <= x <= `EleNum` and x is the chain number) should be when `x == sqrt(EleNum)`?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:564
+  // All Elements can be put inside commoning chains.
+  if (CBucket.ChainSize * ChainNum != EleNum)
+    return false;
----------------
jsji wrote:
> This is common to 552, can we just us different ChainNum w w/o SawSeperator?
Maybe we can revisit this comment after I add more comments to this function to make it clear.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:568
+  // Check each chain has same offsets for all elements.
+  for (unsigned i = 1; i < CBucket.ChainSize; i++)
+    for (unsigned j = 1; j < ChainNum; j++)
----------------
jsji wrote:
> Why we need this check here? 
Suppose there are two chains, we need to make sure, each diff between two adjacent elements is the same among chains.
For example,

after we divided the bucket to two chains A {baseA, offset1, offset2, offset3, offset4}, B {baseB, offset5, offset6, offset7, offset8},
we need to make sure offset1 = offset5, offset2 == offset6, offset3 = offset7, offset4 = offset8.

So we can use two bases and 4 reused offsets.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:613
+  if (MadeChange)
+    for (auto &BB : L->blocks())
+      if (BBChanged.count(BB))
----------------
jsji wrote:
> Can we loop through `BBChanged` only?
yes, I think we can and we should. I will post another patch for this NFC change as this usage is also existing in `updateFormPrep` and `dispFormPrep`


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:630
+  for (unsigned Idx = 1; Idx < Bucket.ChainSize; ++Idx)
+    if (!isSafeToExpand(Bucket.Elements[Idx].Offset, *SE))
+      return MadeChange;
----------------
jsji wrote:
> How often we will get unsafe offsets? Can we just do early exit in the main loop below?
I am not sure how often we get unsafe offset. Before we expand a SCEV, it is safe to call this function first. If necessary, I will do more investigation later to confirm for what kind of case, `isSafeToExpand`is needed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108750