[PATCH] D49007: [PowerPC] Add a peephole post RA to transform the inst that fed by add

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang marked 4 inline comments as done.
steven.zhang added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3103
+  It++;
+  for (;It != E; ++It) {
+    if (!LastDefMI && It->modifiesRegister(Reg, &getRegisterInfo()))
----------------
nemanjai wrote:
> I think the reader has to do a fair bit of thinking to convince themselves that this loop does the right thing. I certainly had to.
> Would it not be easier to just:
> - Exit immediately if a def is found that is not the `DefMI`
> - Break out of the loop if we get to `DefMI` since we don't care what happens prior to it
> 
> The idea would be that we simply cannot transform if we found a register def prior to `DefMI` in the reverse walk. Also, I don't really understand why we care about any other uses of the register being forwarded? If we have shown that the only visible def of the register is `DefMI`, we don't care about any uses - we just need to make sure that if `DefMI->readsRegister(Reg,...)`, we can only transform if `KilledDefMI`.
> So the simplified sequence would be:
> ```
> for (; It != E; ++It) {
>   if (It->modifiesRegister(...) && (&*It) != DefMI)
>     return false;
>   // Made it to DefMI without encountering a clobber.
>   if ((&*It) == DefMI)
>     break;
> }
> 
> // If DefMI also uses the register to be forwarded, we can only forward it if
> // DefMI is being erased.
> if (DefMI->readsRegister(Reg, ...))
>   return KillDefMI;
> return true;
> ```
> 
> Or did I miss something and that code doesn't sufficiently specify the constraints?
You are right. If the DefMI could be erased, there won't any use between the DefMI and MI. We don't need to check the use here. 


https://reviews.llvm.org/D49007