[PATCH] D147532: [MachineLateInstrsCleanup] Improve compile time for huge functions.
Jonas Paulsson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 28 10:18:11 PDT 2023
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
More information about the llvm-commits
mailing list