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

[ChenZheng via Phabricator via llvm-commits]

shchenz marked 6 inline comments as done and 4 inline comments as done.
shchenz added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:1143
       if (MIB->readsRegister(Reg, TRI)) {
-        MachineOperand *MO = 
+        MachineOperand *MO =
             MIB->findRegisterDefOperand(Reg, false, false, TRI);
----------------
jsji wrote:
> This file was changed only by adding or removing whitespace. - Please avoid changing this in this patch.
The empty namespace is added by mistake in this patch's NFC patch https://reviews.llvm.org/rL354438.
But I am ok to remove this change if your insist.


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


================
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
----------------
jsji wrote:
> Is it possible that debug instruction will mess up the logic here? Do we need to skip debug instructions?
debug instructions should not impact functionality of this function. findRegisterUseOperand/findRegisterDefOperand will return nullptr for debug instructions I think. 

But it impacts perf, I have updated.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2444
+    // no use found.
+    if (!IsKillSet) {
+      if ((MO = It->findRegisterUseOperand(RegNo, false, &getRegisterInfo()))) {
----------------
jsji wrote:
> 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?
Yes, this is by design. Before we call this function, we should make sure RegNo liveness is killed in MI.


================
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);
----------------
jsji wrote:
> Is it possible that we have multiple same operands with killed flag set?
Yes, we maybe meet this issue. We should continue to clear not-last-use instruction's killed flag for RegNo.

  // Fixup kill/dead flag after transformation.$
  // Pattern 1:$
  // x = op1 KilledFwdFeederReg, imm$
  // n1 = opn1 KilledFwdFeederReg(killed), regn1$
  // n2 = opn2 KilledFwdFeederReg(killed), regn2$
  // y = op2 0, x$


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


================
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:
> 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?
This is by design. FixupKilledDeadFlags is designed to be a general interface for all PostRA transformations which will violate killed/flags semantics. So it is maybe called like TII->FixupKilledDeadFlags().


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

https://reviews.llvm.org/D58428