[PATCH] D147700: [llvm-exegesis] Allow setting dump file name

Fri Apr 14 02:43:50 PDT 2023

kpdev42 marked an inline comment as done.
kpdev42 added inline comments.

================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.h:67-68
   Expected<Benchmark> runConfiguration(RunnableConfiguration &&RC,
-                                                  bool DumpObjectToDisk) const;
+                                       bool DumpObjectToDisk,
+                                       StringRef DumpFileName) const;

----------------
courbet wrote:
> kpdev42 wrote:
> > courbet wrote:
> > > kpdev42 wrote:
> > > > courbet wrote:
> > > > > Given that `DumpFileName` only makes sense if `DumpObjectToDisk`, use a single `optional<StringRef>` parameter ?
> > > > If we provide just -dump-object-to-disk then llvm-exegesis will use autogenerated name. This is useful when batch opcode testing is used (with -opcode-index=-1). We can use some special file name for this mode, but it looks slightly counter-intuitive. 
> > > > If we provide just -dump-object-to-disk then llvm-exegesis will use autogenerated name
> > > 
> > > Yes, and that would be `{""}`. My point is that we can condense two parameters in one and ensure that we can't write `runConfiguration(RC, false, "file")`, which makes no sense.
> > Oh, sorry, now I understood what you meant. I will update the revision
> I don't see the `optional<StringRef>` :)
Oh, yes. Now it is here )

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147700