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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:236
+      if (MI.modifiesRegister(Reg, TRI)) {
+        MBBDefs.erase(Reg);
+        MBBKills.erase(Reg);
----------------
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.


================
Comment at: llvm/lib/CodeGen/MachineLateInstrsCleanup.cpp:236
+      if (MI.modifiesRegister(Reg, TRI)) {
+        MBBDefs.erase(Reg);
+        MBBKills.erase(Reg);
----------------
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.



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

https://reviews.llvm.org/D147532