[PATCH] D87469: [LLD][ELF] Optimize linker script filename glob pattern matching NFC

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/LinkerScript.cpp:324
+static StringRef getFilename(const InputFile *file) {
+  return file ? file->getNameForScript() : StringRef();
+}
----------------
grimar wrote:
> andrewng wrote:
> > grimar wrote:
> > > It feels that `getFilename` can instead be just inlined now, though?
> > I've inlined the function.
> I've meant it is a single line function and you can probably just remove it and inline its logic.
> E.g:
> ```
>   if (!matchesFileCache || matchesFileCache->first != file)
>     matchesFileCache.emplace(
>         file, filePat.match(getFilename(file ? file->getNameForScript() : "")));
> ```
> Up to you.
> 
> 
> A bit unrelated to this diff: thinking more I wonder if we want to assume an empty file name at all for matching.
> We perhaps can be more explicit. Something like:
> 
> ```
> bool InputSectionDescription::matchesFile(const InputFile *file) const {
>   if (filePat.isTrivialMatchAll())
>     return true;
> 
>   if (!file)
>     return false;
> ...
> ```
> 
> ```
> bool SectionPattern::excludesFile(const InputFile *file) const {
>   if (!file || excludedFilePat.empty())
>     return false;
> ...
> ```
getFilename is called twice. Having it separate looks good to me.

> if (filePat.isTrivialMatchAll())

Avoiding the single-element cache with just `(filePat.isTrivialMatchAll())`?  I think that will not speed up `KEEP (*crtbegin.o(.ctors))` as in GNU ld's internal linker scripts. The idea is, in computeInputSections, when testing sections. The sections of the same file are contiguous. So a single-element cache can speed up.

The description should be modified to mention this point.


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

https://reviews.llvm.org/D87469