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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

I think that this is semantically correct now. There are just a few other stylistic comments.
Also, since this is a rather high-risk patch, can you describe on the patch the functional testing you have done with the patch (list of applications, bootstrap, etc.)?



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3014
+// Check if the 'MI' that has the index OpNoForForwarding 
+// meets the requirement describe in the ImmInstrInfo.
+bool PPCInstrInfo::isUseMIElgibleForForwarding(MachineInstr &MI,
----------------
s/describe/described


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3096
+
+  // Walking the inst reversely(MI-->DefMI) to get the last DEF and USE
+  // of the Reg.
----------------
s/reversely/in reverse


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3103
+  It++;
+  for (;It != E; ++It) {
+    if (!LastDefMI && It->modifiesRegister(Reg, &getRegisterInfo()))
----------------
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?


https://reviews.llvm.org/D49007