[PATCH] D147532: [MachineLateInstrsCleanup] Improve compile time for huge functions.

[Valery Pykhtin via Phabricator via llvm-commits]

vpykhtin added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:236
+      if (MI.modifiesRegister(Reg, TRI)) {
+        MBBDefs.erase(Reg);
+        MBBKills.erase(Reg);
----------------
jonpa wrote:
> jonpa wrote:
> > vpykhtin wrote:
> > > vpykhtin wrote:
> > > > Sorry, I didn't notice this change first.
> > > > 
> > > > You can still use erase by iterator (it's slightly faster than another lookup) because you're using make_early_inc_range.
> > > bad advice - DefI is already incremented after Register Reg = DefI.first. 
> > > 
> > > May be you can leave previous variant of iteration, you can increment the iterator like this:
> > > 
> > > MBBDefs.erase(DefI++) instead of DefI = MBBDefs.erase(DefI)
> > > 
> > > 
> > Yeah - I went for readability at first here as I did not see any change in compile time.
> I did not find anything to convince me 100% that this use of erase with iterator is legal, so I will have to trust you on this ;-)
> 
> I think the readability is ok now as well with this.
> 
Now you're incrementing DefI after erase and in general this is incorrect for STL containers (though it's not the case here) because erase invalidates iterators to erased elements.

MBBDefs.erase(DefI++)  works because arguments are fully evaluated before function call, so this is equivalent to:

auto DefIBefore = DefI;
++DefI; // Now DefI is valid after erase 
MBBDefs.erase(DefIBefore);


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

https://reviews.llvm.org/D147532