[PATCH] D25370: Regenerate removed implicit defs in BranchFolder where necessary

[Matthias Braun via llvm-commits]

MatzeB added a comment.

Hmm BranchFolder::OptimizeImpDefsBlock() has some odd logic indeed, I wonder why nobody found problems with it earlier.

The patch is slightly odd as well. BranchFolder::RegenerateImpDefsInBlock() should be able to restore any missing IMPLICIT_DEFS, so I am not sure why that global set of registers is kept. This also makes me feel uneasy, the approach does not target blocks where IMPLICIT_DEFS were removed (or rather their new predecessors in case they got merged), but instead attempts to fix up any block. (The entry in the ImpDefRegs set may come from a completely different basic block). Nonetheless given that this appears to be broken right now I would not be opposed to the patch when some of the points below are addressed:



================
Comment at: lib/CodeGen/BranchFolding.cpp:160
+                                       std::set<MCPhysReg> &ImpDefRegs) {
+  std::set<unsigned> LocalImpDefRegs;
   MachineBasicBlock::iterator I = MBB->begin();
----------------
What was wrong with SmallSet? SmallSet is usually the better choice over std::set because it provides inline storage and I would consider a small number of elements likely here. Also std::set is ordered and therefore usually not implemented with more efficient hashing.


================
Comment at: lib/CodeGen/BranchFolding.cpp:167-169
       for (MCSubRegIterator SubRegs(Reg, TRI, /*IncludeSelf=*/true);
            SubRegs.isValid(); ++SubRegs)
+        LocalImpDefRegs.insert(*SubRegs);
----------------
Maybe only put "Reg" into the set. And instead does some TRI.regsOerlap() test in the 2nd loop. This may be slightly more efficient anyway as I would expect the tupical unconditional branch has no inputs so there is nothing to test. But the real benefit would be to not have all those subregs in the ImpDefRegs set later.


================
Comment at: lib/CodeGen/BranchFolding.cpp:200-202
+  for (unsigned Reg : LocalImpDefRegs)
+    if (TargetRegisterInfo::isPhysicalRegister(Reg))
+      ImpDefRegs.insert(Reg);
----------------
This should probably not be restricted to physregs. The pass description claims that the pass should also work with virtual regs. (NVPTX and WebAsm are still using virtual registers late in the codegen pipeline).


================
Comment at: lib/CodeGen/BranchFolding.cpp:215
+  for (MachineInstr &MI : *MBB)
+    LiveRegs.stepForward(MI, Tmp);
+
----------------
Using stepForward is discouraged. It will result in overly conservative liveness in case of missing kill flags. If possible go for stepBackward().

Proposal for a different strategy:
Collect live out regs, make a copy of that set
check each instruction backwards:
   check operands:
       on reg define: remove reg from the copy set
       on reg kill: check if reg is still in the copy set, if so append IMPLICIT_DEF at the end
   simulate liveness on the normal set

when reaching the beginning compare liveregs with liveins, for missing regs add IMPLICIT_DEF instruction.


================
Comment at: lib/CodeGen/BranchFolding.cpp:275
 
+  std::set<MCPhysReg> ImpDefRegs;
+
----------------
SmallSet?


================
Comment at: lib/CodeGen/BranchFolding.cpp:302
 
+  for (MachineBasicBlock &MBB : MF)
+    MadeChange |= RegenerateImpDefsInBlock(&MBB, ImpDefRegs);
----------------
All of this can be skipped if ImpDefRegs.empty() or MachineRegisterInfo::tracksLiveness() returns false.


Repository:
  rL LLVM

https://reviews.llvm.org/D25370