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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

Thanks for working on this. Apart from Daniel's suggestion, I have a few more comments but haven't done an in depth analysis of the memory costs/algorithmic properties.

As for performance data, it would be good to have this enabled optionally in the IRTranslator as well. We can compare the effect on compile time and code size in just IRT vs IRT + Legalizer. I would hope we can get a speedup.



================
Comment at: include/llvm/CodeGen/GlobalISel/CSEInfo.h:66
+
+  GISelWorkList<8> TemporaryInsts;
+
----------------
Document what this is for?


================
Comment at: include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h:49
+
+    const MachineBasicBlock *BBA = A->getParent();
+    MachineBasicBlock::const_iterator I = BBA->begin();
----------------
We should assert here the assumption that the iterators from the same block.


================
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)
----------------
'count' is unused here.


================
Comment at: include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h:64
+  MachineInstrBuilder buildCopyIfRequired(MachineInstr *MI, unsigned Dst) {
+    // We need ot build a copy from MI to Dst.
+    return buildCopy(Dst, MI->getOperand(0).getReg());
----------------
Typo 'to'


================
Comment at: include/llvm/CodeGen/GlobalISel/CSEMIRBuilder.h:226
+      return MachineInstrBuilder();
+    FoldingSetNodeID ID;
+    auto Profile = profileMBBOpc(ID, Opc);
----------------
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.


================
Comment at: lib/CodeGen/GlobalISel/CSEInfo.cpp:271
+GISelInstProfileBuilder::addNodeIDRegType(const LLT &Ty) const {
+  uint64_t Val = ((uint64_t)Ty.RawData) << 2 | ((uint64_t)Ty.IsPointer) << 1 |
+                 ((uint64_t)Ty.IsVector);
----------------
Should move this as a helper in LLT, something like serializeData()? That way we don't have to have internal bit layout detail in a user of LLT.


Repository:
  rL LLVM

https://reviews.llvm.org/D52803