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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:67
+//    A1_new = p + base1 // chain 1
+//    B1_new = p + base2 // chain 2, now inside the loop, offset is reused.
+//
----------------
`offset is reused` ? common offset is re-used?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:71
+//      unsigned long x1 = *(unsigned long *)(A1_new + i);
+//      unsigned long x2 = *(unsigned long *)(A1_new + offset + i);
+//      unsigned long x3 = *(unsigned long *)(B1_new + i);
----------------
Should we use `((A1_new + i) + offset )` to be clearer?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:422
+// use that start as a chain separator.
+static bool isValidChainCommoningCandidate(const SCEV *LSCEV) {
+  const SCEVAddRecExpr *ARSCEV = cast<SCEVAddRecExpr>(LSCEV);
----------------
`isChainCommoningCandidate` , to be aligned with `isDSFormCandidate` etc.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:488
+  // Reach the limit.
+  if (Buckets.size() > MaxBucketNum)
+    return;
----------------
Why we have this limit check here, but not in `addOneCandidate`?

Also we should at least emit some debug log if we are really discarding candidate due to bucket limit here.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:491
+
+  if (!isValidChainCommoningCandidate(LSCEV))
+    return;
----------------
This should be checked in collectCandiate, as `isValidCandidate`.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:500
+    const SCEV *Diff = SE->getMinusSCEV(LSCEV, B.BaseSCEV);
+    if (isValidChainCommoningDiff(Diff)) {
+      B.Elements.push_back(BucketElement(Diff, MemI));
----------------
Can we also abstract this check so that we can also reuse `addOneCandidate` to do this.

Something like:

```
 for (auto &B : Buckets) {
    const SCEV *Diff = SE->getMinusSCEV(LSCEV, B.BaseSCEV);
    if ( const auto *Diff = isValidDiff(Diff, LSCEV, B.BaseSCEV)) {
      B.Elements.push_back(BucketElement(Diff, MemI));
      FoundBucket = true;
      break;
    }
  }
```

then  we use `isValidConstantDiff` for others, but `isValidChainCommoningDiff` for this.

```
isValidConstantDiff(Diff, LSCEV, B.BaseSCEV) ={
return dyn_cast<SCEVConstant>(Diff)
}

isValidChainCommoningDiff(Diff, LSCEV, B.BaseSCEV) ={

  assert(LSCEV && "Invalid SCEV for Ptr value.");

  // Don't mess up previous dform prepare.
  if (isa<SCEVConstant>(LSCEV))
    return null_ptr;

  // A single integer type offset.
  if (isa<SCEVUnknown>(LSCEV) && LSCEV->getType()->isIntegerTy())
    return Diff;

  const SCEVNAryExpr *ASCEV = dyn_cast<SCEVNAryExpr>(LSCEV);
  if (!ASCEV)
    return null_ptr;

  for (const SCEV *Op : ASCEV->operands())
    if (!Op->getType()->isIntegerTy())
      return null_ptr;

  return Diff;


}


```


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:585
+  // The number of new bases should both be sqrt(EleNum).
+  if (!SawChainSeparater) {
+    unsigned ChainNum = (unsigned)sqrt(EleNum);
----------------
How about this?

```
 unsigned ChainNum=FirstOffsetReusedCount / FirstOffsetReusedCountInFirstChain;
 if (!SawChainSeparater) 
   ChainNum = (unsigned)sqrt(EleNum);

  CBucket.ChainSize = (unsigned)(EleNum / ChainNum);

  // If this is not a perfect chain(eg: all elements can be put inside
  // commoning chains.), skip now.
  if (CBucket.ChainSize * ChainNum != EleNum)
      return false;

  if (SawChainSeparater) {
  // Check that the offset seqs are the same for all chains?
      for (unsigned i = 1; i < CBucket.ChainSize; i++)
          for (unsigned j = 1; j < ChainNum; j++)
             if (CBucket.Elements[i].Offset !=
                 SE->getMinusSCEV(CBucket.Elements[i + j * CBucket.ChainSize].Offset,
                           CBucket.Elements[j * CBucket.ChainSize].Offset))
             return false;

   }

  for (unsigned i = 0; i < ChainNum; i++)
    CBucket.ChainBases.push_back(CBucket.Elements[i * CBucket.ChainSize]);
```


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:603
+
+  // All Elements can be put inside commoning Chain.
+  if (CBucket.ChainSize * ChainNum != EleNum)
----------------
Not all elements?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:607
+
+  // Check each chain has same offsets for all elements.
+  for (unsigned i = 1; i < CBucket.ChainSize; i++)
----------------
// Check that the offset seqs are the same for all chains?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:948
+
+  if (IsChainCommoning && !EnableChainCommoning)
+    return Buckets;
----------------
Since we are reusing `collectCandidates`, this check can be moved out to around line 1488.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:975
+      if (IsChainCommoning)
+        addOneCandidateForChainCommoning(&J, LSCEV, Buckets, MaxCandidateNum);
+      else if (isValidCandidate(&J, PtrValue, PointerElementType))
----------------
Why we can't pass `isValidChainCommoningCandidate` as `isValidCandate` (yes, need to add one more LSCEV parameter to function prototype) , and pass`addOneCandidateForChainCommoning` as `addOneCandiate` function too?

Then we don't need the `IsChainCommoning` bool parameter and conditional check here.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:498
+
+  for (const auto &BB : L->blocks())
+    for (auto &J : *BB) {
----------------
shchenz wrote:
> 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.
D112196 looks good. But I think we can still do more for this? eg: If both Dform and DQform buckets are empty, then there is no point trying to run this commoning either?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108750