[NFC][PowerPC] Add NFC fixes to PPCInstrinfo.cpp when getting the defined machine instruction.

[amyk]

Summary

This patch adds the following NFC fixes to PPCInstrInfo.cpp when getting the DefMI:

• Fix documentation error to state that we want to flag a use of register between the def and the MI (in post-RA)
• Setting the DefMI to null if the DefMI is neither an LI or and ADDI (while still being in SSA form).

In terms of setting the DefMI to null, this change aims to account for the scenario of when we end up
going through all operands on the machine instruction MI and updating OpNoForForwarding accordingly
once an ADDI is found as the DefMI.

It is possible that once an ADDI is found, we will continue to go through all operands in attempts to find an LI,
but end up looking at every operand until we reach the end if we have not yet found an LI. In the case where the
end is reached and we never end up finding an LI/ADDI, DefMI would be pointing to the last operand of MI
while OpNoForForwarding would still be pointing at the previous ADDI operand found. We reset DefMI to
avoid having DefMI point to an instruction that differs from the one represented by OpNoForForwarding.

[shchenz]

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

3397

If OpNoForForwarding is not changed, then the reassigned DefMI will not be used. Do you find any issue for this?

[maryammo]

other than the question regarding DefMI , LGTM.

[amyk]

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

3397

Thanks for your comment. I believe if OpNoForForwarding remains as ~0U like it is in the beginning in this case, then we did not end up finding a valid operand in pre-SSA form.

I realize that we also anticipate that DefMI would be null as at the end we have,

return OpNoForForwarding == ~0U ? nullptr : DefMI;

Which will indicate that we’ll return a null DefMI if OpNoForForwarding doesn’t end up getting updated.

Thus, I don’t believe this is an issue, but please do let me know if I have misunderstood your question.

[shchenz]

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

3397

I think the change here (reassigning DefMI to nullptr) does not make any functionality change, right? And it makes the code more complicated(at least now one basic block is added), so I was wondering about the benefit about this change.

[amyk]

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

3397

The scenario in mind that served as a motivation for this change is when OpNoForForwarding is changed (as opposed to not being changed). The scenario is described as:

• We'll go through all of the operands on MI and if we end up finding an ADDI as the DefMI, we'll update OpNoForForwarding
• If we've found an ADDI but have not yet found an LI, we will continue to look through all every operand until we reach the end or have found an LI
• It is possible that we may reach the end and never end up finding an LI/ADDI, so DefMI would be pointing to the last operand while OpNoForForwarding would still be pointing at the previous ADDI operand.

An example that @nemanjai brought up to me appears to illustrate this:

       addi 3, 3, 10        // This should be DefMI
       mulld 5, 10, 10
       ori 6, 9, 15         // DefMI (we've stepped through all operands until we reached the end)
       maddld 4, 3, 5, 6    // MI

OpNoForForwarding: 1 (points to the `addi` seen in Operand 1)

We reset the DefMI so that it would not be pointing at an instruction that differs from the OpNoForForwarding that would be originally set.

[shchenz]

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

3397

OK, thanks for explanation, I get your point now. This patch is not a NFC? We are trying to fix some bug here, the bug is not exposed because for now there is no such X/D form instruction that may have addi and li operand at same time starting from operand index 1.

Reassigning DefMI to nullptr seems conservative if the original addi is legal for peephole optimization. There is a addi candidate for peephole, but if the addi operand is not the last operand, the DefMI returned to caller will be set to nullptr with this patch's fix.

How about:

if (Register::isVirtualRegister(TrueReg)) {
  MachineInstr *DefMIForTrueReg = MRI->getVRegDef(TrueReg);
  if (DefMIForTrueReg->getOpcode() == PPC::LI || DefMIForTrueReg->getOpcode() == PPC::LI8 ||
      DefMIForTrueReg->getOpcode() == PPC::ADDI ||
      DefMIForTrueReg->getOpcode() == PPC::ADDI8) {
    OpNoForForwarding = i;
    DefMI = DefMIForTrue;

    // The ADDI and LI operand maybe exist in one instruction at same
    // time. we prefer to fold LI operand as LI only has one Imm operand
    // and is more possible to be converted. So if current DefMI is
    // ADDI/ADDI8, we continue to find possible LI/LI8.
    if (DefMI->getOpcode() == PPC::LI || DefMI->getOpcode() == PPC::LI8)
      break;
  }

[amyk]

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

3397

Thanks for the suggestion! I agree, and thanks for explaining. I like this approach and I also brought this up to discuss with Nemanja. I'll update the patch accordingly with this change, since this will ensure we catch ADDIs that are between the first operand and the last operand, as well.

Also, I want to say for now that this patch will probably remain NFC. This is because ideally, I'd like to have a test case that can not only undergo a transformation, but also has an operand produced by an ADDI, and for this operand to not be the last operand of the MI.

From what I can tell, we are only calling getForwardingDefMI() when we convertToImmediateForm() within the MI peephole optimization pass. There are only two cases within this function where we are able to convert instructions to their immediate forms with ADDI as their DefMI.

Furthermore, it appears that both of these cases (transformToNewImmFormFedByAdd(), and transformToImmFormFedByAdd()) expect the MI to be either a load/store, but these instructions will likely have the ADDI as the last operand since the first operand will be a special zero register. Finding and setting the DefMI to be ADDI is something we already do correctly. For example,

Replacing indexed instruction:
  STDX killed renamable $x3, $zero8, renamable $x4 :: (store (s64) into %ir._param_data, align 1)
Fed by:
  renamable $x4 = ADDI8 $x1, 68
With:
  STD killed renamable $x3, 68, $x1 :: (store (s64) into %ir._param_data, align 1)

Thus, I probably won't add a test case and will keep the NFC label, as this would currently be NFC and would prevent a possible future bug. Feel free to let me know if you have any additional thoughts on this.

[amyk]

Apply @shchenz's suggestion to this patch.

[shchenz]

Thanks very much for addressing this potential bug. LGTM.

nit: I saw there was a typo fix for flat to flag in the first revision, just a reminder. Maybe you have other plan for that change?

[amyk]

In D137483#3973000, @shchenz wrote:

Thanks very much for addressing this potential bug. LGTM.

nit: I saw there was a typo fix for flat to flag in the first revision, just a reminder. Maybe you have other plan for that change?

Yes, good point! I'm not sure what happened to it when I updated the patch... but yes, I intended to include it. I'll update the patch/commit with the typo update, as well. Thank you!

[amyk]

Add back the typo fix that I accidentally removed in the last revision.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp

if (MRI->isSSA()) {
     for (int i = 1, e = MI.getNumOperands(); i < e; i++) {
       if (!MI.getOperand(i).isReg())
         continue;
       Register Reg = MI.getOperand(i).getReg();
       if (!Register::isVirtualRegister(Reg))
         continue;
       Register TrueReg = TRI->lookThruCopyLike(Reg, MRI);
       if (Register::isVirtualRegister(TrueReg)) {
-        DefMI = MRI->getVRegDef(TrueReg);
-        if (DefMI->getOpcode() == PPC::LI || DefMI->getOpcode() == PPC::LI8 ||
-            DefMI->getOpcode() == PPC::ADDI ||
-            DefMI->getOpcode() == PPC::ADDI8) {
+        MachineInstr *DefMIForTrueReg = MRI->getVRegDef(TrueReg);
+        if (DefMIForTrueReg->getOpcode() == PPC::LI ||
+            DefMIForTrueReg->getOpcode() == PPC::LI8 ||
+            DefMIForTrueReg->getOpcode() == PPC::ADDI ||
+            DefMIForTrueReg->getOpcode() == PPC::ADDI8) {
           OpNoForForwarding = i;
+          DefMI = DefMIForTrueReg;
           // The ADDI and LI operand maybe exist in one instruction at same
           // time. we prefer to fold LI operand as LI only has one Imm operand
           // and is more possible to be converted. So if current DefMI is
           // ADDI/ADDI8, we continue to find possible LI/LI8.
           if (DefMI->getOpcode() == PPC::LI || DefMI->getOpcode() == PPC::LI8)
             break;
         }
       }
     }
   } else {
     // Looking back through the definition for each operand could be expensive,
     // so exit early if this isn't an instruction that either has an immediate
     // form or is already an immediate form that we can handle.
     ImmInstrInfo III;
     unsigned Opc = MI.getOpcode();
     bool ConvertibleImmForm =
if ((Opc == PPC::OR || Opc == PPC::OR8) &&
         MI.getOperand(1).getReg() == MI.getOperand(2).getReg())
       return nullptr;
     for (int i = 1, e = MI.getNumOperands(); i < e; i++) {
       MachineOperand &MO = MI.getOperand(i);
       SeenIntermediateUse = false;
       if (MO.isReg() && MO.isUse() && !MO.isImplicit()) {
         Register Reg = MI.getOperand(i).getReg();
         // If we see another use of this reg between the def and the MI,
-        // we want to flat it so the def isn't deleted.
+        // we want to flag it so the def isn't deleted.
         MachineInstr *DefMI = getDefMIPostRA(Reg, MI, SeenIntermediateUse);
         if (DefMI) {
           // Is this register defined by some form of add-immediate (including
           // load-immediate) within this basic block?
           switch (DefMI->getOpcode()) {
           default:
             break;
           case PPC::LI: