[PATCH] D122118: [MachineCopyPropagation] Eliminate spillage copies that might be caused by eviction chain

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi @lkail,

Thanks for your patience.

Looks mostly good to me.

The only thing that makes me uneasy is the potential impact on compile time of `CheckCopyConstraint`.
Do you know on average how many chains we see per function?

I know we have a bot tracking compile time. If that doesn't show anything significant, I guess we could enable it by default. I just don't know how extensive the tests are.

Alternatively, to avoid surprising everybody, we could add a way to enable/disable the new folding.
Either like what was done with `UseCopyInstr` or with a target hook (you can add a command line option too that would override what the target asked for for testing purposes).

Then only enabled it for your target and send an RFC asking people to try the new folding for their targets.

What do you think?

Cheers,
-Quentin



================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1113
+  // we put the COPY in this set to keep property#2.
+  DenseSet<MachineInstr *> CopySourceInvalid;
+
----------------
Nit: `const` on `MachineInstr*`


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1133
+        // If violate property#2, we don't fold the chain.
+        for (size_t I = 1, N = SC.size(); I < N; ++I) {
+          if (CopySourceInvalid.count(SC[I]))
----------------
You should be able to use a range loop:
```
for (const MachineInstr *Spill : SC) {
    if (CopySourceInvalid.count(Spill))
            return;
}
```


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1138
+
+        for (size_t I = 0, N = RC.size() - 1; I < N; ++I) {
+          if (CopySourceInvalid.count(RC[I]))
----------------
Nit: range loop


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1144
+        auto CheckCopyConstraint = [this](Register Def, Register Src) {
+          for (const TargetRegisterClass *RC : TRI->regclasses()) {
+            if (RC->contains(Def) && RC->contains(Src))
----------------
That's going to be pretty expensive to walk all the register classes.
I'm guessing you're trying to check if the resulting copy is legal and unfortunately there's no good way to do that.

Did you see that showing up in compile time profile?



================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1174
+
+        for (size_t I = 1; I < SC.size() - 1; ++I) {
+          SC[I]->eraseFromParent();
----------------
Nit: range loop


================
Comment at: llvm/lib/CodeGen/MachineCopyPropagation.cpp:1183
+      return false;
+    auto CopyOperands = isCopyInstr(MaybeCopy, *TII, UseCopyInstr);
+    if (!CopyOperands)
----------------
Nit: Here and other places where you use `isCopyInstr`: use the explicit type instead of `auto`. (The return type is hard to infer.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122118