[PATCH] D55028: [gn build] Add template for running llvm-tblgen and use it to add build file for llvm/lib/IR.

[Nico Weber via Phabricator via llvm-commits]

thakis marked 3 inline comments as done.
thakis added a comment.

Thanks for the review!



================
Comment at: llvm/utils/gn/build/run_tablegen.py:8
+# Prefix with ./ to run built binary, not arbitrary stuff from PATH.
+sys.exit(subprocess.call(['./' + sys.argv[1]] + sys.argv[2:]))
----------------
phosek wrote:
> [Chromium version](https://chromium.googlesource.com/chromium/src/+/master/build/gn_run_binary.py#23) does more fancy error checking on Windows, shall we do the same?
That's somewhat recent (https://chromium.googlesource.com/chromium/src/+/8c50b3ed049c736406943ac24dc40fc8ac1e782c). Since this script is only used to run tblgen, shorter code seemed preferable over the small utility gain.


================
Comment at: llvm/utils/gn/secondary/llvm/utils/TableGen/tablegen.gni:27
+
+template("tablegen") {
+  assert(defined(invoker.args), "args must be defined for $target_name")
----------------
phosek wrote:
> Have you considered copying Chromium's [`compiled_action.gni`](https://chromium.googlesource.com/chromium/src/+/master/build/compiled_action.gni) and then implementing `tablegen` in terms of that? It seems generally useful as I suspect you'll need the same functionality for Clang tablegen as well in the future.
I did consider it! Right now this is only used for tablegen though, and for upstreaming I figured I'd make clang_tblgen call this template and make tblgen_target something the invoker can set, and then clang_tblgen would look like (pseudocode):

template("clang_tblgen") {
  tablegen($target_name) {
    tblgen_target = "//clang/utils/TableGen:clang-tblgen"
  }
}

(in my prototyping branch, clang-tblgen is just a copy-pasted version of this file so I'm not 100% sure if this is going to pan out, but I think it should.)


================
Comment at: llvm/utils/gn/secondary/llvm/utils/TableGen/tablegen.gni:37
+  action(target_name) {
+    forward_variables_from(invoker, [ "visibility" ])
+
----------------
phosek wrote:
> You should probably forward `testonly` as well`.
I haven't needed it yet -- are you aware of any test-only tblgen invocations?


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

https://reviews.llvm.org/D55028