[PATCH] D115053: [Bazel] Switch LLVM targets based on configuration flags.

[Geoffrey Martin-Noble via Phabricator via llvm-commits]

GMNGeoffrey added a comment.

This overall seems like an improvement. Some thoughts:

1. Being able to just list the targets you want seems good. Do we also have a use case for "everything except target X" though? That would be hard to express in this scheme, whereas with individual target enable/disable flags it would be easy. Is there a way to get both? Is it worth it?
2. It would be nice to be able to have targets that are configured, but are disabled by default. I guess we can add that pretty easily by creating a separate variable and using that for `build_setting_default` in the string list flag.
3. It's a bit hard to guarantee that nothing directly references the targets. I think we do want it to be possible to directly reference a specific target, but if someone passes a flag without a target enabled, it seems like we shouldn't build that target. As I suggest above, perhaps we should make the srcs of disabled targets empty.
4. As a test, can you try building with no targets enabled? I'm curious what happens. It might also flush out edge cases. What about with only some weird target enabled (and no X86)



================
Comment at: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel:30
 
-enum_targets_gen(
-    name = "targets_def_gen",
-    src = "include/llvm/Config/Targets.def.in",
-    out = "include/llvm/Config/Targets.def",
-    macro_name = "TARGET",
-    targets = llvm_targets,
+# A command-line flag for restricting the list of LLVM targets to build.
+string_list_flag(
----------------
Can you include an example usage, as you have in the patch description here?


================
Comment at: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel:46
 
-enum_targets_gen(
-    name = "asm_printers_def_gen",
-    src = "include/llvm/Config/AsmPrinters.def.in",
-    out = "include/llvm/Config/AsmPrinters.def",
-    macro_name = "ASM_PRINTER",
-    targets = llvm_target_asm_printers,
-)
-
-# Enabled targets with ASM parsers.
-llvm_target_asm_parsers = [
-    t
-    for t in llvm_targets
-    if glob(["lib/Target/{}/AsmParser/CMakeLists.txt".format(t)])
-]
-
-enum_targets_gen(
-    name = "asm_parsers_def_gen",
-    src = "include/llvm/Config/AsmParsers.def.in",
-    out = "include/llvm/Config/AsmParsers.def",
-    macro_name = "ASM_PARSER",
-    targets = llvm_target_asm_parsers,
-)
-
-# Enabled targets with disassemblers.
-llvm_target_disassemblers = [
-    t
-    for t in llvm_targets
-    if glob(["lib/Target/{}/Disassembler/CMakeLists.txt".format(t)])
-]
-
-enum_targets_gen(
-    name = "disassemblers_def_gen",
-    src = "include/llvm/Config/Disassemblers.def.in",
-    out = "include/llvm/Config/Disassemblers.def",
-    macro_name = "DISASSEMBLER",
-    targets = llvm_target_disassemblers,
-)
-
-# Enabled targets with MCA.
-llvm_target_mcas = [
-    t
-    for t in llvm_targets
-    if glob(["lib/Target/{}/MCA/CMakeLists.txt".format(t)])
+# Replace macros in headers with lists of enabled targets.
+[
----------------
When I first read this I thought you were talking about Starlark macros. Maybe that could be clarified?


================
Comment at: utils/bazel/llvm-project-overlay/llvm/BUILD.bazel:1775
         name = target["name"] + "Info",
         srcs = ["lib/Target/" + target["name"] + "/TargetInfo/" + target["name"] + "TargetInfo.cpp"],
         hdrs = glob(["lib/Target/" + target["name"] + "/TargetInfo/*.h"]),
----------------
Perhaps we should select out all srcs if the target isn't enabled. Then there's no chance some other dependency is going to pull in anything from a disabled target


================
Comment at: utils/bazel/llvm-project-overlay/llvm/enum_targets_gen.bzl:5
 
 """A rule to expand LLVM target enumerations.
 
----------------
Please update the documentation


================
Comment at: utils/bazel/llvm-project-overlay/llvm/enum_targets_gen.bzl:56
+            mandatory = True,
+            doc = "A list of LLVM targets that are valid for this enum.",
+        ),
----------------
I had to read the code to understand what this meant. Can you expand the documentation a bit? It is probably also worth explaining *why* you need two parameters here rather than just passing the targets you want (because this is dependent on selects and therefore has to be processed within a rule).


================
Comment at: utils/bazel/llvm-project-overlay/llvm/target_macros.bzl:35-38
+    result = selects[0]
+    for s in selects[1:]:
+      result += s
+    return result
----------------
Just seems a bit cleaner


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115053