[PATCH] D122615: Early return in constrainOperandRegClass()

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:61-65
+  // Check if the register class is already as required.
+  auto *OldRegClass = MRI.getRegClassOrNull(Reg);
+  if (OldRegClass == &RegClass)
+    return Reg;
+
----------------
I'm not keen on duplicating internals of constrainRegToClass() behaviour (even obvious behaviours like this one). Do you get a similar perf fix if you check between the possible change and the notification with something like this?:
```
  auto *OldRegClass = MRI.getRegClassOrNull(Reg);
  Register ConstrainedReg = constrainRegToClass(MRI, TII, RBI, Reg, RegClass);
  if (ConstrainedReg != Reg) {
    insert copy
    notify observers
  } else {
    auto *NewRegClass = MRI.getRegClassOrNull(Reg);
    if (OldRegClass != &NewRegClass) {
      notify observers
    }
  }
```




================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:97
       }
       Observer->changingAllUsesOfReg(MRI, Reg);
       Observer->finishedChangingAllUsesOfReg();
----------------
It's not for this patch but this call is supposed to happen before the change. It won't work properly for all observers if it's after the change. For example, the debug location info checking one will compare the state after the change to the state after the change with this call where it currently is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122615