[PATCH] D31821: Remove redundant copy in recurrences

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a reviewer: wmi.
MatzeB added a comment.

I'd like to hear @qcolombet's opinion about this, also +wmi who wrote the similar `isRevCopyChain()`.

This patch
----------

- Nitpicks below
- Do you have an idea of the compiletime impact of this patch?
- I am slightly worried about the use of MachineLoopInfo: Is it really necessary here? Maybe the same can be done with some simpler dominance check? Can you tell when the MachineLoopInfo is/isn't available (as that will impact whether this rule is applied or not).

General Observations
--------------------

I am not happy with the general approach of throwing more and more patterns at TwoAddressInstructions:

- Yes, LLVMs handling of TwoAddressInstruction as a pre-RA pass is a bad idea IMO (and I don't know why it was done this way): We are unnecessarily constraining the allocation problem and don't necessarily do a good job upfront without knowing how the allocation will work out.
- This pass adds another pattern at TwoAddressInstruction: It is similar to `isRevCopyChain()` but slightly more complicated so we end up with 160 extra lines of analysis for yet another pattern. If this trend continues we will have an even harder time maintaining this pass.
- On the other hand this improves code quality today without rearchitecting the code.



================
Comment at: lib/CodeGen/TwoAddressInstructionPass.cpp:187
     AU.addPreservedID(MachineDominatorsID);
+    AU.addPreserved<MachineLoopInfo>();
     MachineFunctionPass::getAnalysisUsage(AU);
----------------
This makes no sense! You are starting to use the machine loop info, all you do here is mark it preserved a 2nd time; There was already addPreservedID(MachineLoopInfoID) which had the same effect.


================
Comment at: lib/CodeGen/TwoAddressInstructionPass.cpp:577
+MachineInstr* TwoAddressInstructionPass::findReachingDefInMBB(
+    MachineOperand *UseMO) {
+  assert(UseMO->isUse() && UseMO->isReg());
----------------
Use references for things that cannot be nullptr. Similar for a few other places here.


================
Comment at: lib/CodeGen/TwoAddressInstructionPass.cpp:579-580
+  assert(UseMO->isUse() && UseMO->isReg());
+  auto Inst = UseMO->getParent();
+  auto Reg = UseMO->getReg();
+
----------------
Don't use `auto` when the type isn't immediately obvious just by looking at the line. (`auto` is unfriendly towards the readers of your code). Similar in a few more places.


================
Comment at: lib/CodeGen/TwoAddressInstructionPass.cpp:585
+  MI = std::next(MI);
+  for ( ; MI != MBB->rend() ; ++MI)
+    if ((*MI).findRegisterDefOperandIdx(Reg) != -1)
----------------
we don't tend to have a space before the ';' here. Did you try clang-format on your patch?


================
Comment at: lib/CodeGen/TwoAddressInstructionPass.cpp:621
+
+    auto OpsNum = DefMI->getDesc().getNumOperands();
+    auto SrcOpIdx = DefMI->getDesc().getNumDefs();
----------------
This only gives you the declared/minimum number of operands, there may be more.


================
Comment at: lib/CodeGen/TwoAddressInstructionPass.cpp:714-715
+    for (auto &Def: MRI->def_instructions(UD->getOperand(1).getReg())) {
+      if (MLI->getLoopFor(Def.getParent()) != Loop)
+        continue;
+
----------------
Isn't this check superfluous? I would expect this to be true anyway if the instruction is inside `MBB` which is checked later.


================
Comment at: test/CodeGen/X86/twoaddr-recurrence.ll:1-2
+; RUN: llc < %s -march=x86-64 | FileCheck %s
+;
+; CHECK: bb7
----------------
Did you try to create a .mir test so the pass can be tested in isolation? If yes and it didn't work, could you tell us why so we can improve the .mir testing.


https://reviews.llvm.org/D31821