In D150880#4538498, @thopre wrote:

Ping?

Ping?

Address review comments

Address review comments

Simplify signature since it does not throw any error

• Split APInt change into separate patches and only keep changes to support arbitrary precision
• use [[maybe_unused]] instead of (void)

In D150880#4448939, @arichardson wrote:

Overall LGTM but as this is a rather large diff I may have missed something.

Ping?

In D150880#4383688, @thopre wrote:

Ping?

Ping?

In D150878#4362820, @dblaikie wrote:

Do you need someone to commit this for you? (if so, what name & email address would you like to be used to attribute the work?)

In D150878#4361073, @thopre wrote:

Remove <64 bit APInt consumeInteger tests

Remove <64 bit APInt consumeInteger tests

In D150879#4353625, @lattner wrote:

I would suggest a silly microoptimization:

static const char BothDigits[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz";
const char *Digits = BothDigits+(UpperCase ? 0 : 36);

Avoids looking up two different GOT entries.

In D150797#4352145, @jhenderson wrote:

Given that there are unit tests, this code is in the library's public-facing API. Does removing the error check cause problems if users happen to be leveraging the FileCheck library in some manner?

As mentioned, I went for a change in how tensorflow uses a local LLVM repo instead: https://github.com/tensorflow/tensorflow/commit/64a1638818af21c33280287a95ea88ea62ea9237

In D143295#4121029, @chapuni wrote:

I supposed llvm_configure is the only public entry for users.

In D143295#4110430, @aaronmondal wrote:

I'll have to agree with @chapuni though, Bazel has exceptional support for bash scripts etc, anything that could be expressed in starlark with genrules and bash scripts seems much better to me than python. E.g. overlaying/moving files is like 10x faster in bash than it is in python.

Also, removing the python dependency would potentially make the build much more portable. I don't think this commit would be the right place to do this and I understand that pytorch/tensorflow devs may prefer using python in general.

However, I think the current functionality of the script is not something where python is actually needed. I'm not sure where else we'd need python for the LLVM build, but I'd say that requiring a rather large python dependency in every CI image is not very economical when we already have starlark and bash.

In D143295#4109933, @chapuni wrote:

Excuse me, I am not a fan of it.
Could you remain vars.bzl stuff, unless python would have interest to vars?

I am planning for vars.bzl to give configurations to build rules (and repository rules).

• Address black's linting errors
• Call overlay code in overlay_directories directly instead of using a subprocess

Oops sorry, I'm not sure how it happened. I thought I copy/pasted new code only.

Use LLVM_VERSION as a CMake variable and LLVM_VERSION_* as substitutions

In D138274#3938218, @arsenm wrote:

I thought we had this already. The amount of boiler plate required repeated in each component is depressing, but I wouldn't be surprised if it's really needed

Rewrite CHECK lines

Modify tests as per suggestion and checked it shows a difference with and without diff

Remove IR block in test

LGTM, thanks!