[PATCH] D52803: [GISel]: Add support for CSEing continuously during GISel passes

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar marked an inline comment as done.
aditya_nandakumar added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/CSEInfo.h:42
+/// new instructions as well as deletions. It however will not be able to
+/// track instruction mutations. In such cases, recordNewInstruction should be
+/// called (for eg inside MachineIRBuilder::recordInsertion).
----------------
dsanders wrote:
> I'm not keen on using recordNewInstruction to report changes to existing instructions. We should have another interface for mutations
I can in a separate patch change the API of MachineIRBuilder to reflect the different kinds of recordings that we may need and then update this patch to use that.


================
Comment at: include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h:51
+    MachineBasicBlock::const_iterator I = BBA->begin();
+    unsigned count = 0;
+    for (; &*I != A && &*I != B; ++I, ++count)
----------------
aemerson wrote:
> 'count' is unused here.
This was used to profile by printing which domination algorithm would be preferable - I updated it now.


================
Comment at: include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h:226
+      return MachineInstrBuilder();
+    FoldingSetNodeID ID;
+    auto Profile = profileMBBOpc(ID, Opc);
----------------
aemerson wrote:
> There's a fair amount of code duplication here. This might get a bit religious depending on your philosophy w.r.t macros, but we could factor out most of the code from the first part of the builder methods. I'm not going to say it should definitely be done, I think it's just about worth it though.
Thanks. I also want to avoid this code duplication. For now, I've kept it simple, but it's definitely something that needs to be addressed here using whatever mechanism.


================
Comment at: lib/CodeGen/GlobalISel/CSEInfo.cpp:37
+  // instructions.
+  MF.setDelegate(this);
+}
----------------
dsanders wrote:
> It seems that this persists until GISelCSEInfo is destroyed which could be many passes from now. Effectively this imposes behaviour changes on subsequent passes that may not be expecting it.
> 
> In the case where a subsequent pass gets it but doesn't intend to preserve it, the subsequent pass is going to pay the cost of the maintenance unnecessarily. In the case where a subsequent pass gets it and intends to preserve it but needs to do so carefully, it's going to be fighting against a default maintenance method that was installed without its knowledge.
> 
> Another problem is that GISelCSEInfo doesn't have exclusive ownership of the MF delegate. If a pass wants to use it, this is going to clobber the one they set up.
> 
> In summary, I think passes need to install their own strategy to maintain it. This could be the standard one but I think they need to choose it.
I've made this explicit now - the passes can use the RAIIDelegateInstaller now to set this up.


Repository:
  rL LLVM

https://reviews.llvm.org/D52803