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

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineRegisterInfo.h:176
 
-  void resetDelegate(Delegate *delegate) {
+  Delegate *getDelegate(DelegateReceiver DR) {
+    if (TheDelegates.find(DR) == TheDelegates.end())
----------------
qcolombet wrote:
> 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.
Will remove the enum class and can use just SmallPtrSet.


================
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) &&
----------------
qcolombet wrote:
> Rename this in `addDelegate` and ditch the `DelegateReceiver` argument.
Sure


================
Comment at: llvm/lib/CodeGen/MachineRegisterInfo.cpp:151
+  for (auto &TheDelegate : TheDelegates)
+    TheDelegate.second->MRI_NoteNewVirtualRegister(Reg);
+
----------------
qcolombet wrote:
> 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? 
It makes sense to notify just like the earlier code. I will move it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134950