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

[Amy Kwan via Phabricator via llvm-commits]

amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3394
+        } else
+          DefMI = nullptr;
       }
----------------
shchenz wrote:
> amyk wrote:
> > shchenz wrote:
> > > amyk wrote:
> > > > shchenz wrote:
> > > > > If `OpNoForForwarding` is not changed, then the reassigned `DefMI` will not be used. Do you find any issue for this?
> > > > 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.
> > > 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.
> > 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. 
> 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;
>         }
> ```
> 
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 `ADDI`s 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137483