[PATCH] D134950: [CodeGen] Use delegate to notify targets when virtual registers are created

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

I think we're going in the right direction, thanks for your patience with all this.



================
Comment at: llvm/include/llvm/CodeGen/MachineRegisterInfo.h:84
   MachineFunction *MF;
-  Delegate *TheDelegate = nullptr;
+  SmallDenseMap<DelegateReceiver, Delegate *, 1> TheDelegates;
 
----------------
I would go with something simpler: `SmallPtrSet<Delegate *>`.


================
Comment at: llvm/include/llvm/CodeGen/MachineRegisterInfo.h:176
 
-  void resetDelegate(Delegate *delegate) {
+  Delegate *getDelegate(DelegateReceiver DR) {
+    if (TheDelegates.find(DR) == TheDelegates.end())
----------------
I don't think we need that method and as a result we can ditch the `DelegateReceiver` altogether.

Essentially when an event occurs, we need to notify all the delegates.

If you still need a getter for your target specific use case, you can keep a hold on your custom delegate in your custom `MachineFunctionInfo` class.

Alternatively, I guess we could introduce the notion of ID (what you did with `DelegateReceiver`) but that must not be an enum. For instance, we could use something similar to Pass::ID for that, but for now if the `MachineFunctionInfo` solution works for you, let's not spend too much time on that.

For the record, the rationale for being against the enum is if people use more than one custom delegate, then the developer's experience is not great because we would have only one ID (RESERVED) to "register" it or they would need to modify the `DelegateReceiver` to add enum values.


================
Comment at: llvm/include/llvm/CodeGen/MachineRegisterInfo.h:186
+    // delegate first unattaches itself.
+    assert(getDelegate(DR) == delegate &&
+           "Only an existing delegate ca0n perform reset!");
----------------
After you ditch the `DR`, we would just need to check that the given `delegate` is already in the set.


================
Comment at: llvm/include/llvm/CodeGen/MachineRegisterInfo.h:191
 
-  void setDelegate(Delegate *delegate) {
-    assert(delegate && !TheDelegate &&
-           "Attempted to set delegate to null, or to change it without "
+  void setDelegate(Delegate *delegate, DelegateReceiver DR) {
+    assert(delegate && !TheDelegates.count(DR) &&
----------------
Rename this in `addDelegate` and ditch the `DelegateReceiver` argument.


================
Comment at: llvm/lib/CodeGen/MachineRegisterInfo.cpp:151
+  for (auto &TheDelegate : TheDelegates)
+    TheDelegate.second->MRI_NoteNewVirtualRegister(Reg);
+
----------------
By moving this call here, the delegate may miss some important information (like the register class).
I wonder if it would make sense to send two "notifications": one when creating the incomplete register, like here, and one when the register is fully done (like what you did with the clone method).

Otherwise, we'll probably need to stick to having to send the notification just once where it originally was (i.e., within `MachineRegisterInfo::createVirtualRegister`, `MachineRegisterInfo::createGenericVirtualRegister`, and `MachineRegisterInfo::cloneVirtualRegister`).

What do you think? 


================
Comment at: llvm/lib/CodeGen/MachineRegisterInfo.cpp:179
   if (TheDelegate)
-    TheDelegate->MRI_NoteNewVirtualRegister(Reg);
+    TheDelegate->MRI_NotecloneVirtualRegister(Reg, VReg);
   return Reg;
----------------
We'll need to notify all delegates here, not just the `RESERVED` one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134950