Use explicit std::string conversions instead of implicit llvm::StringRef->std::string conversions to prepare Glow for builds using LLVM >=11

[opti-mix]

LLVM >= 11 made llvm::StringRef->std::string conversion explicit, because this conversion is pretty expensive and developers should be aware that it happens.

The result of this change is that a lot of existing code relying on this implicit conversion is now broken and does not compile anymore. Glow has a lot of such broken places in its source code due to this.

Most of such places using a llvm::StringRef sr should be mechanically changed into std::string(sr). In some cases, it may make sense to use const std::string &s instead of llvm::StringRef sr.

[ankushduacodes]

@opti-mix I would love to work on this. This is my first issue on this repo, Please let me know if I should be aware of anything else. 🚀

[opti-mix]

@ankushduacodes

I would love to work on this. This is my first issue on this repo, Please let me know if I should be aware of anything else.

It is great you'd like to work on this!

Since there are a lot of places in Glow's code base using the old StringRef API, many of them may need to be changed. It could be a good idea to submit a number of smaller PRs to address this, instead of one huge PR. This will also make it easier to review.

[ankushduacodes]

@opti-mix Could you please give me a starting point as to where should I start refactoring.

[opti-mix]

Try to configure CMake to use LLVM 11, assuming you installed it on your system. On MacOS it would be via "brew install llvm"

Use the following command:

PATH=/usr/bin:/usr/local/bin CC=/usr/local/opt/llvm/bin/clang cmake -G Ninja -DCMAKE_CXX_COMPILER=/usr/local/opt/llvm/bin/clang++ -DCMAKE_PREFIX_PATH=/usr/local/opt/llvm -DCMAKE_BUILD_TYPE=Debug -DGLOW_WITH_CPU=ON -DGLOW_WITH_LLVMIRCODEGEN=ON -DGLOW_WITH_OPENCL=OFF -DGLOW_WITH_CPU=ON /your/path/to/Glow

Once it is configured, run ninja and look for compilation errors complaining about StringRef and std::string. Start fixing them file by file.

[sparkingdark]

is this open for work @opti-mix

[ankushduacodes]

I am sorry couldn't venture out a pr, i have been quite busy, But I will be working on it soon @opti-mix

[NinaPacifier]

Is this open for work @opti-mix?? I would love to work on this.

[opti-mix]

@NinaPacifier Nobody has started working on this yet and no PRs were submitted. So, it is open for grabs. Please refer to this issue in your PRs, so that others see that it is being worked on.

[jfix71]

Hi @NinaPacifier -- I actually think @ayermolo is already working on this, and will probably upstream it soon. Just want to make sure you don't waste your time on it.

[ayermolo]

@NinaPacifier Sorry didn't mean to take it over. There is still an issue with ORC v1 being deprecated in latest llvm. https://llvm.org/docs/ORCv2.html#transitioning-orcv1-to-orcv2

[opti-mix]

While @ayermolo fixed a bunch of places, there is still a lot of other places to fix. So, it is still for grabs!

How to reproduce:

On MacOS Catalina I tried to run the following commands:

# Needed in my case to workaround some issues with /usr/include introduced by Apple
export SDKROOT="/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/"

# Configure using Xcode's llvm/clang (which reports LLVM 12). -DCMAKE_PREFIX_PATH=/usr/local/opt/llvm is the path to LLVM 11 installed using brew.
PATH=/usr/bin:/usr/local/bin  cmake -G Ninja  -DCMAKE_PREFIX_PATH=/usr/local/opt/llvm  -DCMAKE_BUILD_TYPE=Debug -DGLOW_WITH_CPU=ON -DGLOW_WITH_LLVMIRCODEGEN=ON -DGLOW_WITH_OPENCL=ON   -DGLOW_WITH_CPU=ON ~/src/Glow

# Run a build
ninja

If you run these or equivalent commands you'll see many more places where llvm::StringRef->std::string issue pops up.

[opti-mix]

After looking at it a bit more, I'm starting to dislike the mess created by using llvm::StringRef in some places and const std::string & or std::string in other places. I don't like the difference as such and also the fact that it looks syntactically very different.

So, I wonder if we should consider introducing some type aliases, e.g. for const std::string &. Something like: using StdStringRef = const std::string &. Then at least parameters declarations in functions will look like void f(glow::StdStringRef s);, which is more syntactically similar to void f(llvm::StringRef s);

Another thing I'm considering: Maybe instead of touching a lot of places that need llvm::StringRef->std::string conversion, we should rather introduce a replacement for llvm::StringRef that is almost 100% compatible to it, but does support an implicit conversion to std::string just like llvm::StringRef used to do before?

E.g. we could have something like this class:

namespace glow {
/// Glow's version of llvm::StringRef. It is meant to be an almost drop-in
/// replacement for llvm::StringRef. The main difference from the recent
/// versions of llvm::StringRef is the support for implicit conversions to
/// std::string, which eliminates the need to update all the places effected by
/// the deprecation of this conversion in LLVM.
class StringRef : public llvm::StringRef {
public:
  using llvm::StringRef::const_iterator;
  using llvm::StringRef::iterator;
  /// Import all constructors from llvm::StringRef.
  using llvm::StringRef::StringRef;
  /// Constructors.
  StringRef(llvm::StringRef s) : llvm::StringRef(s) {}
  StringRef(const std::string &s) : llvm::StringRef(s) {}
  template <int T>
  StringRef(const llvm::SmallString<T> &s) : StringRef(s.str()) {}
  template <int T> StringRef(llvm::SmallString<T> s) : StringRef(s.str()) {}
  /// Allow for an implicit conversion to std::string.
  operator std::string() const { return str(); }
};
}

And then we can global search/replace and replace llvm::StringRef by glow::StringRef in our Glow codebase. Somewhat similar to glow::make_unique that we introduced to hide differences in std::unique support introduced by LLVM.

Is it better/safer than updating all the affected places in Glow that rely on an implicit conversion to std::string?

@jfix71 @ayermolo WDYT?

[ayermolo]

My bad, it was building on my linux server, and service that glow as dependency was getting passed string issue during a build. There is still a question of deprecated ORC v1 with llvm 12.

Personally I would lean towards cleaning up interfaces and usage of llvm::StringRef and std::string, maybe going with the StdStringRef, and updating the coding guide. Using the glow defined StringRef seems like going backwards and re-introducing implicit conversions, and adds another data structure that developers need to figure out when to use.

[chintanaprabhu]

@opti-mix I came across this issue while working on migrating my school research project to LLVM 11. I would be happy to work on this issue and lead towards a fix. Please let me know if there are any specifics to be kept in consideration before submitting PRs.

[saranyakrish14]

@opti-mix We got the need for building glow with LLVM11 and came across this issue. We fixed it in bunch of places and were able to built and test successfully. Can we push a PR with the fix?

[opti-mix]

@saranyakrish14 @chintanaprabhu If you have fixes please submit PRs and we'll review them! Thanks!

[slokeshrestha26]

@opti-mix is this open to work on? I am new to contributing. Please let me know if it is a good first issue. Thank you.

[srsonia2506]

My first attempt at open source contribution, A little rusty with C++ and looking to get back into it. Would this be a good first issue for me to handle?