[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:66
+//
+//    A1’ = p + base1 // chain 1
+//    B1’ = p + base2 // chain 2, now inside the loop, offset is reused.
----------------
Can we avoid using `'` in examples?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:417
+
+  // A single pointer. We can treat it with offset 0.
+  if (isa<SCEVUnknown>(Start) && Start->getType()->isPointerTy())
----------------
treat it with? treat it as?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:492
+SmallVector<Bucket, 16>
+PPCLoopInstrFormPrep::collectCandidatesForChainCommoning(Loop *L) {
+  SmallVector<Bucket, 16> Buckets;
----------------
Why this can not reuse `collectCandidates`?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:498
+
+  for (const auto &BB : L->blocks())
+    for (auto &J : *BB) {
----------------
We already collected both Dform and DQform buckets before, can we reuse some info to do early exit?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:526
+  const SCEV *Offset = CBucket.Elements[1].Offset;
+  unsigned TotalCount = 1;
+  unsigned FirstGroupCount = 1;
----------------
`TotalCount`? `ReusableOffsetCount`?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:527
+  unsigned TotalCount = 1;
+  unsigned FirstGroupCount = 1;
+  unsigned EleNum = CBucket.Elements.size();
----------------
`FirstGroupCount`? NewGroupCount?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:529
+  unsigned EleNum = CBucket.Elements.size();
+  bool SawSeparater = false;
+  for (unsigned j = 2; j != EleNum; ++j) {
----------------
`SawSeperator`? `SawDifferentOffset`?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:531
+  for (unsigned j = 2; j != EleNum; ++j) {
+    if (SE->getMinusSCEV(CBucket.Elements[j].Offset,
+                         CBucket.Elements[j - 1].Offset) == Offset) {
----------------
Do we have assumption about the order of Element offset? Can we add comments


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:541
+  // No reuseable offset, skip this bucket.
+  if (TotalCount == 1)
+    return false;
----------------
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?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:544
+
+  // All elements are increased by Offset.
+  // The number of new bases should both be sqrt(EleNum).
----------------
// Only one offset?


================
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) {
----------------
Why `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;
----------------
This is common to 552, can we just us different ChainNum w w/o SawSeperator?


================
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++)
----------------
Why we need this check here? 


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:605
+    // There is benefit because of reuse of offest 'X'.
+    if (Bucket.Elements.size() < 4)
+      continue;
----------------
Can we introduce an option to control, and just set 4 to be the default?

Also, why we can't push this check into `prepareBasesForCommoningChains`?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:613
+  if (MadeChange)
+    for (auto &BB : L->blocks())
+      if (BBChanged.count(BB))
----------------
Can we loop through `BBChanged` only?


================
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;
----------------
How often we will get unsafe offsets? Can we just do early exit in the main loop below?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:648
+
+  SCEVExpander SCEVE(*SE, Header->getModule()->getDataLayout(), "pistart");
+
----------------
`pistart`? Can we use a more meaningful name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108750