[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:
> > > 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137483