[PATCH] D58428: [PowerPC] fix killed/dead flag after reg+reg to reg+imm transformation

[Jinsong Ji via Phabricator via llvm-commits]

jsji requested changes to this revision.
jsji added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1143
       if (MIB->readsRegister(Reg, TRI)) {
-        MachineOperand *MO = 
+        MachineOperand *MO =
             MIB->findRegisterDefOperand(Reg, false, false, TRI);
----------------
This file was changed only by adding or removing whitespace. - Please avoid changing this in this patch.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2421
+// register RegNo.
+void PPCInstrInfo::fixUpKillFlags(MachineInstr &MI, MachineInstr &DefMI,
+                                  unsigned RegNo) const {
----------------
We not only fixup `Kill` flag, but also set `Dead` flag, so maybe we should update the name?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2436
+
+  // Walking the inst in reverse order (MI<->DefMI).
+  MachineBasicBlock::reverse_iterator It = MI;
----------------
```(MI<->DefMI)```
->
```(MI -> DefMI]```



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2439
+  MachineBasicBlock::reverse_iterator E = MI.getParent()->rend();
+  It++;
+  MachineOperand *MO = nullptr;
----------------
Why `It++;` here? Can we  add comments to explain?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2441
+  MachineOperand *MO = nullptr;
+  for (; It != E; ++It) {
+    // If killed is not set, set killed for its use or set dead for its def if
----------------
Is it possible that debug instruction will mess up the logic here? Do we need to skip debug instructions?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2444
+    // no use found.
+    if (!IsKillSet) {
+      if ((MO = It->findRegisterUseOperand(RegNo, false, &getRegisterInfo()))) {
----------------
If we don't set `IsKillSet` for `MI` in line 2433, is it by design to also allow updating `kill/dead` for `RegNo` here? eg: we call this function with a `RegNo` that is not used in `MI`, and last use before `MI` is not killed too?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2459
+      // If killed is already set, clear old killed flag.
+      if ((MO = It->findRegisterUseOperand(RegNo, true, &getRegisterInfo()))) {
+        MO->setIsKill(false);
----------------
Is it possible that we have multiple same operands with killed flag set?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2487
+  bool KillFwdDefMI = !SeenIntermediateUse && IsForwardingOperandKilled;
+  unsigned ForwardOperandReg = MI.getOperand(ForwardingOperand).getReg();
   if (KilledDef && KillFwdDefMI)
----------------
Why `ForwardOperandReg` instead of `ForwardingOperandReg` here?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3353
 
+  // Get Killed info in case fixup needed after transformation.
+  unsigned ForwardKilledOperandReg = ~0U;
----------------
Killed -> killed


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3410
 
   LLVM_DEBUG(dbgs() << "With:\n");
   LLVM_DEBUG(MI.dump());
----------------
Should we dump the instruction after flag fix?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3476
 
+  // Get Killed info in case fixup needed after transformation.
+  unsigned ForwardKilledOperandReg = ~0U;
----------------
Killed -> killed


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:414
                               MachineInstr **KilledDef = nullptr) const;
+  void fixUpKillFlags(MachineInstr &MI, MachineInstr &DefMI, unsigned RegNo) const;
   void replaceInstrWithLI(MachineInstr &MI, const LoadImmediateInfo &LII) const;
----------------
Can we add some doxygen comments here ? Especially relationship between `MI` and `RegNo`.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:414
                               MachineInstr **KilledDef = nullptr) const;
+  void fixUpKillFlags(MachineInstr &MI, MachineInstr &DefMI, unsigned RegNo) const;
   void replaceInstrWithLI(MachineInstr &MI, const LoadImmediateInfo &LII) const;
----------------
jsji wrote:
> Can we add some doxygen comments here ? Especially relationship between `MI` and `RegNo`.
Also, why this needs to be "Public" here? Is it intended for reuse somewhere else?


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

https://reviews.llvm.org/D58428