[PATCH] D38616: [RegisterCoalescer] Don't set read-undef in pruneValues, only clear

[Mikael Holmén via Phabricator via llvm-commits]

uabelho added inline comments.


================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:2688-2692
               if (MO.getSubReg() != 0)
-                MO.setIsUndef(EraseImpDef);
+                // Only set undef on the MO if we can't find any reaching
+                // definition of the value. If it does exist and we set undef on
+                // the MO we will effectively kill the reaching definition.
+                MO.setIsUndef(EraseImpDef && !LR.getVNInfoBefore(Def));
----------------
MatzeB wrote:
> uabelho wrote:
> > MatzeB wrote:
> > > I wonder if we shouldn't change it to this instead which:
> > > ```
> > >               if (MO.getSubReg() != 0 && MO.isUndef() && !EraseImpDef)
> > >                 MO.setIsUndef(false);
> > > ```
> > > it's simpler, matches the comment in front of the for and it seems to pass the x86 tests at least...
> > But this means we will never set isUndef, we will only clear it? So the stuff I talk about in the commit message that we sometimes need to introduce undef, if we've removed an IMPLICIT_DEF, isn't really true? In those cases maybe there would already be undef on the MO?
> > 
> > I suppose the simplified code works if
> >  (MO.getSubReg() != 0) && !MO.isUndef() && EraseImpDef && !LR.getVNInfoBefore(Def)
> > can never happen?
> > 
> > I ran a bunch of tests with 
> > 
> >               assert(!((MO.getSubReg() != 0) && !MO.isUndef() && EraseImpDef && !LR.getVNInfoBefore(Def)) &&
> >                      "Unexpected need of read-undef on subreg write!");
> >               if (MO.getSubReg() != 0 && MO.isUndef() && !EraseImpDef)
> >                 MO.setIsUndef(false);
> > 
> > and I didn't see the assert blowing and everything went well so maybe it's indeed what we want to do.
> > 
> > I'll update the patch to your suggestion!
> The only way we can have a partial write and nothing live before is when the write is marked undef. So we must already have all the undef flags we need.
> 
> Or coming from another angle coalescing should only increase the range of live intervals so we should only need to erase some undef flags but never add new ones.
Great! Thank you!


Repository:
  rL LLVM

https://reviews.llvm.org/D38616