[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

Sami Tolvanen via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jul 30 12:20:12 PDT 2021


samitolvanen added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:45
+  }
+  return true;
+}
----------------
nickdesaulniers wrote:
> samitolvanen wrote:
> > nickdesaulniers wrote:
> > > Can llvm::any_of or llvm::none_of be used here?
> > > llvm/ADT/STLExtras.h
> > Maybe, but I don't see how they would make this function any cleaner. Did you have something specific in mind?
> Something like?
> 
> return any_of(Name, [](const char &C) { return isAlnum(C) || C == '_' || C == '.'; }
> 
> or maybe we need !none_of(...)? (not sure if characters of a string can be enumerated this way)
Since we want to ensure all the characters in the string match the predicate, I believe it would make sense to use `all_of()` instead.

However, I don't see the point of introducing additional complexity to such a trivial function to shave off a couple of lines of code. While it might not actually matter here, using `all_of()` also seems to generate ~5x as many instructions to execute:

https://godbolt.org/z/Pndfxj6rM

If you feel like the current version is too long, I can drop one line by changing the loop to use:
```
if (!isAlnum(C) && C != '_' && C != '.')
  return false;
```
I initially wanted to keep the test identical to `MCAsmInfo::isAcceptableChar()` to make it easier to see it actually matches, but since it's no longer identical, I suppose that doesn't matter.

Thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104058



More information about the cfe-commits mailing list