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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:45
+  }
+  return true;
+}
----------------
samitolvanen wrote:
> 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?
> using all_of() also seems to generate ~5x as many instructions to execute:

Hard to argue with that.

Might be nice to add some test coverage of this code with an mscv target triple though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104058