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

[Jinsong Ji via Phabricator via llvm-commits]

jsji accepted this revision as: jsji.
jsji added a comment.

LGTM.



================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:240
 char PPCLoopPreIncPrep::ID = 0;
 static const char *name = "Prepare loop for pre-inc. addressing modes";
 INITIALIZE_PASS_BEGIN(PPCLoopPreIncPrep, DEBUG_TYPE, name, false, false)
----------------
Should change the name here, we are doing more than pre-inc. now.


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:440
+
 // TODO: implement a more clever base choosing policy.
 // Currently we always choose an exist load/store offset. This maybe lead to
----------------
`TODO:` -> `FIXME:`


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:518
+  // For some DS form load/store instructions, it can also be an update form,
+  // if the stride is a multipler of 4. Use update form if prefer it.
+  bool CanPreInc = (Form == UpdateForm ||
----------------
`multipler` -> `multiple`


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:514
+
+  const SCEV *BasePtrStartSCEV = nullptr;
+  if (!SE->isLoopInvariant(BasePtrSCEV->getStart(), L))
----------------
Why not move this down to line 522?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:515
+  const SCEV *BasePtrStartSCEV = nullptr;
+  if (!SE->isLoopInvariant(BasePtrSCEV->getStart(), L))
+    return MadeChange;
----------------
Seems non-relevant change, why moving this after `BasePtrIncSCEV` check?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:559
+  Instruction *PtrInc = nullptr;
+  Instruction *NewBasePtr;
+  if (CanPreInc) {
----------------
Why not init it to `nullptr` as well?


================
Comment at: llvm/lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:587
+      Instruction *InsPoint = BB->getTerminator();
+      PtrInc = GetElementPtrInst::Create(
+          I8Ty, NewPHI, BasePtrIncSCEV->getValue(),
----------------
Can we add some comments about why we need to create new GEP at the end of every predecessor BB here?


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

https://reviews.llvm.org/D67088