[PATCH] D17293: [MC] AsmLexer: 30% speedup on tests, added extensible identifier's character set support.

[Pykhtin, Valery via llvm-commits]

> -----Original Message-----
> From: Rafael Espíndola [mailto:rafael.espindola at gmail.com]
> Sent: Tuesday, March 01, 2016 10:45 PM
> To: reviews+D17293+public+fca97e390e37f5d8 at reviews.llvm.org
> Cc: Pykhtin, Valery; Jim Grosbach; Arsenault, Matthew; Daniel Dunbar; Tom
> Stellard; llvm-commits; nhaustov at gmail.com
> Subject: Re: [PATCH] D17293: [MC] AsmLexer: 30% speedup on tests, added
> extensible identifier's character set support.
> 
> On 1 March 2016 at 14:34, Valery Pykhtin <valery.pykhtin at amd.com> wrote:
> > vpykhtin added inline comments.
> >
> > ================
> > Comment at: include/llvm/MC/MCParser/MCAsmLexer.h:211
> > @@ +210,3 @@
> > +  /// allow/disallow an identifier to contain specified characters
> > + virtual void setIdentifierCharSet(bool Value,
> > +                                    StringRef PfxCharSet,
> > ----------------
> > rafael wrote:
> >> Why do you need to make these virtual?
> > Well not making it virtual would require bitvector sets to be part of this
> class. I'm not objecting though as it already done with SkipSpace and
> AllowAtInIdentifier.
> 
> Yes, please make it non virtual.
Ok.
> 
> > ================
> > Comment at: include/llvm/MC/MCParser/MCAsmLexer.h:216
> > @@ +215,3 @@
> > +  /// test whether the specified character can be found in an
> > + identifier  virtual bool isIdentifierCharSetContains(char) const =
> > + 0;
> > +
> > ----------------
> > rafael wrote:
> >> is..Contains is a strange name since it has two verbs.
> > What would be a better name here?
> 
> Just isAllowedChar?
Nice, may be isAllowedIDChar?

> 
> 
> > Well it based on my previuos experience on Windows where we had lexer
> using these routines eating up to 10% of scan time. Probably not so
> "generally" as I stated though. I'm not insisting on this particular change and
> can remove it.
> 
> Yes, please leave it out. It can be done as an independent patch.
> 
> BTW, how exactly have you benchmarked this?

Ran all MC\*.* tests several times and measured time. To be honest I didn’t benchmarked individual contribution of  isDigit change. It mostly benefits from single "in bitvector" check.

> 
> Cheers,
> Rafael