[PATCH] D143617: [Clang][CMake] Support perf, LBR, and Instrument CLANG_BOLT options

[Petr Hosek via Phabricator via cfe-commits]

phosek added inline comments.


================
Comment at: clang/utils/perf-training/bolt.lit.cfg:12
+if config.clang_bolt_mode.lower() == "instrument":
+    clang_binary = "clang-bolt.inst"
+else:  # perf or LBR
----------------
This name would ideally be passed through the generated `lit.site.cfg` rather than hardcoding it here.


================
Comment at: clang/utils/perf-training/perf-helper.py:75
+    parser.add_argument(
+        "--lbr", required=False, action="store_true", help="Use perf with branch stacks"
+    )
----------------
This could be omitted.


================
Comment at: clang/utils/perf-training/perf-helper.py:85
+    opts = parser.parse_args(args[:last_arg_idx])
+    # cmd = shlex.split(args[last_arg_idx:])
+    cmd = args[last_arg_idx:]
----------------
Can we remove this?


================
Comment at: clang/utils/perf-training/perf-helper.py:88-97
+    perf_args = []
+    perf_args.extend(
+        (
+            "perf",
+            "record",
+            "--event=cycles:u",
+            "--freq=max",
----------------
Why not assign to `perf_args` directly?


================
Comment at: clang/utils/perf-training/perf-helper.py:115
+    )
+    parser.add_argument("p2b_path", help="Path to llvm-bolt")
+    parser.add_argument("path", help="Path containing perf.data files")
----------------
I'd call it `llvm-bolt` for simplicity.


================
Comment at: clang/utils/perf-training/perf-helper.py:118-120
+    parser.add_argument(
+        "--nolbr", required=False, action="store_true", help="Use -nl perf2bolt mode"
+    )
----------------
I'd consider providing both `--lbr` and `--no-lbr` options.


================
Comment at: clang/utils/perf-training/perf-helper.py:123-126
+    p2b_args = []
+    p2b_args.extend(
+        (opts.p2b_path, opts.binary, "--aggregate-only", "--profile-format=yaml")
+    )
----------------
Why not assign to `p2b_args` directly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143617