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

[Matthias Braun via llvm-commits]

MatzeB added a comment.

In https://reviews.llvm.org/D25370#565145, @kparzysz wrote:

> In https://reviews.llvm.org/D25370#565130, @MatzeB wrote:
>
> > 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.
>
>
> It didn't start that way, but in some Thumb testcase it produced an implicit def of LR, which caused some CHECK-NEXT line to fail (implicit defs show up in comments in the final assembly).  I thought LR would be reserved on ARM, but it's not (apparently deliberately) and I didn't want to touch anything there.  That is really the only motivation for this global set.


A register may be non-allocatable but still not reserved, we do track liveness for those registers.
This seems like an example for the reservations I had: Was the pass cleaning up something that was broken before anyway which I maybe should have left alone? Was the IMPLICIT_DEF in that case legit so we should rather adjust the CHECKs. Having a global set seems like a bad idea to restrict this maybe a legit IMPLICIT_DEF of LR was removed in another block...

> 
> 
>> 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 reason is that the anticipated optimizations could do just about anything with the rest of the code: merge block, split blocks, duplicate, etc. I thought that keeping track of what happened to what block would be too convoluted and too error-prone.

I understand but it doesn't convince me that the course taken is a good one :)



================
Comment at: lib/CodeGen/BranchFolding.cpp:160
+                                       std::set<MCPhysReg> &ImpDefRegs) {
+  std::set<unsigned> LocalImpDefRegs;
   MachineBasicBlock::iterator I = MBB->begin();
----------------
kparzysz wrote:
> MatzeB wrote:
> > 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.
> SmallSet does not provide iterators: that was the reason I used set.
Bummer. LLVM also features DensetSet which I would still consider a better choice than std::set here where we don't need a stable order.


================
Comment at: lib/CodeGen/BranchFolding.cpp:200-202
+  for (unsigned Reg : LocalImpDefRegs)
+    if (TargetRegisterInfo::isPhysicalRegister(Reg))
+      ImpDefRegs.insert(Reg);
----------------
kparzysz wrote:
> MatzeB wrote:
> > 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).
> I use LivePhysRegs to establish available registers.  It does not work with virtual registers.
I just realized that block live in lists only contain PhysRegs right now. NVPTX and WebAsm both seem to have liveness tracking disabled past regalloc. So the physreg check can be an `assert()` instead.


Repository:
  rL LLVM

https://reviews.llvm.org/D25370