Page MenuHomePhabricator

gn build: add RISCV target
ClosedPublic

Authored by dlj on May 10 2019, 10:20 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

dlj created this revision.May 10 2019, 10:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2019, 10:20 PM

Awesome, thanks! Just one real question, the last comment below:

llvm/utils/gn/secondary/llvm/lib/Target/RISCV/BUILD.gn
30 ↗(On Diff #199130)

I think this is the first target that has Utils depending on the higher-up tablegen target. What is this needed for?

llvm/utils/gn/secondary/llvm/lib/Target/RISCV/MCTargetDesc/BUILD.gn
4 ↗(On Diff #199130)

Can you do https://reviews.llvm.org/D61859 for this file?

llvm/utils/gn/secondary/llvm/lib/Target/RISCV/Utils/BUILD.gn
11 ↗(On Diff #199130)

That's a bit unfortunate :/ I think ARM has this too, but it's a bit yucky. It'd be nicer if BaseInfo wouldn't depend on MCTargetDesc's tablegen internals.

llvm/utils/gn/secondary/llvm/lib/Target/targets.gni
26 ↗(On Diff #199130)

(pass -U9999 to git diff when generating diffs, so that phab doesn't have to say "Context not available.")

44 ↗(On Diff #199130)

Since nothing currently reads this, consider not having this variable (and giving the else below an empty body). But up to you, doesn't matter much.

Actually, "the first comment below" – phab showed them in a different order in the preview. I meant this comment: https://reviews.llvm.org/D61821#inline-549166

dlj updated this revision to Diff 199368.May 13 2019, 9:20 PM
dlj marked 4 inline comments as done.
dlj added inline comments.May 13 2019, 9:23 PM
llvm/utils/gn/secondary/llvm/lib/Target/RISCV/BUILD.gn
30 ↗(On Diff #199130)

Oh, you're completely right... it should go in the public_deps of Utils. (Now I see what you mean... that's what AArch64 does, too.)

llvm/utils/gn/secondary/llvm/lib/Target/targets.gni
26 ↗(On Diff #199130)

Oops, sorry about that...

thakis added inline comments.May 14 2019, 5:48 AM
llvm/utils/gn/secondary/llvm/lib/Target/RISCV/BUILD.gn
30 ↗(On Diff #199130)

Thanks for moving RISCVGenSystemOperands to Utils/BUILD.gn. Does Utils still need depend on this tablegen group() here? If so, why? (No other target currently has this dep, as far as I can tell.) If not, can you remove the dep and make LLVMRISCVCodeGen dep on the tablegen targets in this file directly (like in other targets)?

dlj updated this revision to Diff 199451.May 14 2019, 8:16 AM
dlj marked 2 inline comments as done.
dlj updated this revision to Diff 199453.May 14 2019, 8:19 AM
dlj added inline comments.
llvm/utils/gn/secondary/llvm/lib/Target/RISCV/BUILD.gn
30 ↗(On Diff #199130)

Ah, yes I missed that. The dep isn't needed in Utils... however, it was hiding a couple of other missing deps (from AsmParser and MCTargetDesc).

dlj updated this revision to Diff 199603.May 15 2019, 6:54 AM

Run gn format

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.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 12 2019, 5:39 AM
Closed by commit rL363154: gn build: add RISCV target (authored by nico). · Explain Why
This revision was automatically updated to reflect the committed changes.