[PATCH] D123394: [CodeGen] Late cleanup of redundant address/immediate definitions.

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:47
+  // Data structures to map regs to their definitions per MBB.
+  using Reg2DefMap = std::map<Register, MachineInstr*>;
+  using MBB2RegDefsMap = std::map<MachineBasicBlock *, Reg2DefMap>;
----------------
craig.topper wrote:
> Can these be DenseMaps?
I did make some experiments with DenseMap and other alternatives earlier (see May 16 above) and never got any improvements in compile time by doing so.

In addition, it currently seems unwise to do that since processBlock ("Clear any entries in map that MI clobbers") is looping over the entries while erasing some of them. I could not do this now even after rewriting the loop like:


```
  // Data structures to map regs to their definitions per MBB.
-  using Reg2DefMap = std::map<Register, MachineInstr*>;
-  using MBB2RegDefsMap = std::map<MachineBasicBlock *, Reg2DefMap>;
+  using Reg2DefMap = DenseMap<Register, MachineInstr*>;
+  using MBB2RegDefsMap = DenseMap<MachineBasicBlock *, Reg2DefMap>;
   MBB2RegDefsMap RegDefs;
 
   // Set of visited MBBs.
@@ -257,11 +258,10 @@ bool MachineLateInstrsCleanup::processBlock(MachineBasicBlock *MBB) {
 
     // Clear any entries in map that MI clobbers.
     for (auto DefI = MBBDefs.begin(); DefI != MBBDefs.end();) {
-      Register Reg = DefI->first;
+      Reg2DefMap::iterator CurrI = DefI++;
+      Register Reg = CurrI->first;
       if (MI->modifiesRegister(Reg, TRI))
-        DefI = MBBDefs.erase(DefI);
-      else
-        ++DefI;
+        MBBDefs.erase(CurrI);
     }

```

I then got "Assertion `isHandleInSync() && "invalid iterator access!"' failed.", so it seems to me that this isn't supported.


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:52
+  // Set of visited MBBs.
+  MBBSet Visited;
+
----------------
craig.topper wrote:
> Every MachineBasicBlock has a number assigned to it. This could possibly be a BitVector using the basic block numbering. See `MachineBasicBlock::getNumber()` and `MachineFunction::getNumBlockIDs`
> 
> You might also be able to use the numbering instead of a map for MBB2RegDefsMap. But probably depends on how spare that map is.
Using BitVector for Visited and Visited_preds seems to lower the average compile time (on the files it shows up) by about ~0.04%, so it looks like a very slight but noticeable improvement.

For the MBB2RegDefsMap I saw a similar slight further improvement on average, so with both of these changes an average improvement of 0.1% seems to result. It may be that with this change (MBB2RegDefsMap) some of the slower cases get slower, but the average is improved.

I have only tested this on SystemZ, but I guess a BitMap really should be faster than a map, so it makes sense to use it to me.



================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:222
+      }
+      if (AllSame) {
+        MBBDefs[Reg] = DefMI;
----------------
craig.topper wrote:
> Could this use llvm::all_of with a lambda?
yeah


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:281
+
+  std::list<MachineBasicBlock *> Worklist;
+  Worklist.push_back(PendingMBB);
----------------
craig.topper wrote:
> Would std::queue be better here?
It's 0.01% faster on average, so why not :-)



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

https://reviews.llvm.org/D123394