[PATCH] D105872: [PowerPC] prepare more loop load/store instructions

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:172
     PPCTargetMachine *TM = nullptr;
-    const PPCSubtarget *ST; 
+    const PPCSubtarget *ST;
     DominatorTree *DT;
----------------
Please commit these unrelated formatting changes directly then rebase. Thanks.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:539
+  if (!IsConstantInc && Form == UpdateForm) {
+    LLVM_DEBUG(dbgs() << "not a constant incresement for update form!\n");
+    return MadeChange;
----------------
`incresement` typo?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:545
   // if the stride is a multipler of 4. Use update form if prefer it.
-  bool CanPreInc = (Form == UpdateForm ||
-                    ((Form == DSForm) && !BasePtrIncSCEV->getAPInt().urem(4) &&
-                     PreferUpdateForm));
+  bool CanPreInc =
+      (Form == UpdateForm ||
----------------
Update comments to describe the forms we can prepare now. 


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:755
+                                                const SCEV *BasePtrIncSCEV) {
+  if (isa<SCEVConstant>(BasePtrIncSCEV))
+    return cast<SCEVConstant>(BasePtrIncSCEV)->getValue();
----------------
Add comments about early exits, please.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:771
+
+  auto getExistingNode = [&](Instruction *I) -> Value * {
+    Value *StrippedBasePtr = I;
----------------
Comments about this lambda please.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:776
+      // time considering.
+      if (BC->hasOneUser())
+        StrippedBasePtr = *BC->users().begin();
----------------
Not sure why we need this check? Do you have examples that may cost us compile time?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:782
+
+    Instruction *StrippedI = dyn_cast<Instruction>(StrippedBasePtr);
+    if (!StrippedI)
----------------
Can these two whiles be merged into one similar to:

```
      while ((BC = dyn_cast<Instruction>(StrippedBasePtr)) &&
             BC->getOpcode() == Instruction::BitCast && BC->hasOneUser )
        StrippedI = BC->getOperand(0);
```


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:821
+
+    if (CurrentPHINode->getNumIncomingValues() == 2) {
+      if ((CurrentPHINode->getIncomingBlock(0) == LatchBB &&
----------------
Why not use early exit here?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:825
+          (CurrentPHINode->getIncomingBlock(1) == LatchBB &&
+           CurrentPHINode->getIncomingBlock(0) == PredBB)) {
+        if (PHIBasePtrIncSCEV == BasePtrIncSCEV)
----------------
Why not use early exit here?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopInstrFormPrep.cpp:829
+            if (Instruction *I = dyn_cast<Instruction>(User))
+              if (Value *IncNode = getExistingNode(I))
+                return IncNode;
----------------
Recursive call?! I think we normally don't use recursive code in LLVM. Please convert it into a worklist. Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105872