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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

I haven't gone through the patch in full but I have a few comments based on the discussion we had today



================
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).
----------------
I'm not keen on using recordNewInstruction to report changes to existing instructions. We should have another interface for mutations


================
Comment at: lib/CodeGen/GlobalISel/CSEInfo.cpp:37
+  // instructions.
+  MF.setDelegate(this);
+}
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D52803