[PATCH] D61821: gn build: add RISCV target

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Sorry for the long wait here, I wanted to patch this in and look at it some. I've finally gotten around to this.

If it's ok, I'd like to request two changes:

1. (minor) I think LLVMRISCVCodeGen needs a dep on Utils
2. The reason the RISCV:tablegen target is needed is because RISCVGenCompressInstEmitter is specific to RISCV – the rest is like the other targets. So I'd suggest we call out in a comment this target being special, and only give it increased visibility, instead of having a tablegen target.

Full proposed diff:

  $ git diff
  diff --git a/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/AsmParser/BUILD.gn b/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/AsmParser/BUILD.gn
  index 34aba6c3426..ef28b23aca3 100644
  --- a/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/AsmParser/BUILD.gn
  +++ b/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/AsmParser/BUILD.gn
  @@ -13,7 +13,7 @@ static_library("AsmParser") {
       "//llvm/lib/MC",
       "//llvm/lib/MC/MCParser",
       "//llvm/lib/Support",
  -    "//llvm/lib/Target/RISCV:tablegen",
  +    "//llvm/lib/Target/RISCV:RISCVGenCompressInstEmitter",
       "//llvm/lib/Target/RISCV/MCTargetDesc",
       "//llvm/lib/Target/RISCV/Utils",
     ]
  diff --git a/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/BUILD.gn b/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/BUILD.gn
  index 5fff59e36f0..2b9b90935de 100644
  --- a/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/BUILD.gn
  +++ b/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/BUILD.gn
  @@ -1,45 +1,39 @@
   import("//llvm/utils/TableGen/tablegen.gni")
  
  +# RISCV is the only target that has a "compress instr emitter", and it's
  +# a bit strange in that it defines static functions depending on which
  +# defines are set. Instead of housing these functions in one library,
  +# various libraries include the generated .inc file with different defines set.
   tablegen("RISCVGenCompressInstEmitter") {
  -  visibility = [ ":tablegen" ]
  +  visibility = [
  +    ":LLVMRISCVCodeGen",
  +    "AsmParser",
  +    "MCTargetDesc",
  +  ]
     args = [ "-gen-compress-inst-emitter" ]
     td_file = "RISCV.td"
   }
  
   tablegen("RISCVGenDAGISel") {
  -  visibility = [ ":tablegen" ]
  +  visibility = [ ":LLVMRISCVCodeGen" ]
     args = [ "-gen-dag-isel" ]
     td_file = "RISCV.td"
   }
  
   tablegen("RISCVGenMCPseudoLowering") {
  -  visibility = [ ":tablegen" ]
  +  visibility = [ ":LLVMRISCVCodeGen" ]
     args = [ "-gen-pseudo-lowering" ]
     td_file = "RISCV.td"
   }
  
  -# This should contain tablegen targets generating .inc files included
  -# by other targets. .inc files only used by .cpp files in this directory
  -# should be in deps on the static_library instead.
  -group("tablegen") {
  -  visibility = [
  -    ":LLVMRISCVCodeGen",
  -    "AsmParser",
  -    "MCTargetDesc",
  -    "Utils",
  -  ]
  -  public_deps = [
  +static_library("LLVMRISCVCodeGen") {
  +  deps = [
       ":RISCVGenCompressInstEmitter",
       ":RISCVGenDAGISel",
       ":RISCVGenMCPseudoLowering",
  -  ]
  -}
  -
  -static_library("LLVMRISCVCodeGen") {
  -  deps = [
  -    ":tablegen",
       "MCTargetDesc",
       "TargetInfo",
  +    "Utils",
       "//llvm/include/llvm/Config:llvm-config",
       "//llvm/lib/CodeGen",
       "//llvm/lib/CodeGen/AsmPrinter",
  diff --git a/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/MCTargetDesc/BUILD.gn b/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/MCTargetDesc/BUILD.gn
  index a44b898b635..09ca1808651 100644
  --- a/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/MCTargetDesc/BUILD.gn
  +++ b/llvm/utils/gn/secondary/llvm/lib/Target/RISCV/MCTargetDesc/BUILD.gn
  @@ -55,7 +55,7 @@ static_library("MCTargetDesc") {
       ":RISCVGenMCCodeEmitter",
       "//llvm/lib/MC",
       "//llvm/lib/Support",
  -    "//llvm/lib/Target/RISCV:tablegen",
  +    "//llvm/lib/Target/RISCV:RISCVGenCompressInstEmitter",
       "//llvm/lib/Target/RISCV/Utils",
     ]
     include_dirs = [ ".." ]

Does that sound ok to you? With that, lgtm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61821