[PATCH] D85809: [Remarks][1/2] Expand remarks hotness threshold option support in more tools

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Options.td:546
+  "Minimum profile count required for an optimization remark to be output">,
+  MetaVarName<"<number>">;
 def opt_remarks_passes: Separate<["--"], "opt-remarks-passes">,
----------------
MaskRay wrote:
> `<integer>` might be preciser.
This is not done


================
Comment at: llvm/include/llvm/LTO/Config.h:128
+  /// The threshold is an Optional value, which maps to one of the 3 states:
+  /// 1. 0            => threshold disabled. All emarks will be printed.
+  /// 2. positive int => manual threshold by user. Remarks with hotness exceed
----------------
Does swapping the meaning of 0 and None make more sense? `None` looks like a better default value.


================
Comment at: llvm/include/llvm/Remarks/HotnessThresholdParser.h:47
+public:
+  HotnessThresholdParser(cl::Option &O) : cl::parser<Optional<uint64_t>>(O) {}
+
----------------
Regarding the MSVC compiliation issue. If you use llvm::Optional here, does it work?


================
Comment at: llvm/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll:12
 
+; Check low threshold allows remarks to emit
+; RUN: rm -f %t.t300.yaml
----------------
Full stop.


================
Comment at: llvm/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll:30
+; RUN:           -r %t.bc,main,px -o %t.o %t.bc
+; RUN: not FileCheck %s -check-prefix=YAML < %t.t301.yaml
+
----------------
We don't use `not FileCheck`. If you want to check a pattern does not exist, use https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-not-directive


================
Comment at: llvm/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll:53
+; RUN:           -r %t.bc,patatino,px \
+; RUN:           -r %t.bc,main,px -o %t.o %t.bc 2>&1 | not FileCheck %s
+
----------------
Please fix the not FIleCheck


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85809