This is an archive of the discontinued LLVM Phabricator instance.

[exegesis] "Skip codegen" dry-run mode
ClosedPublic

Authored by lebedev.ri on Dec 27 2022, 10:53 AM.

Details

Summary

While "skip measurements mode" is super useful for test coverage,
i've come to discover it's trade-offs. It still calls back-end
to actually codegen the target assembly, and that is what is taking
80%+ of the time regardless of whether or not we skip the measurements.

On the other hand, just being able to see that exegesis can come up
with a snippet to measure something, is already very useful,
and takes maybe a second for a all-opcode sweep.

Diff Detail

Event Timeline

lebedev.ri created this revision.Dec 27 2022, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 27 2022, 10:53 AM
Herald added a subscriber: mstojanovic. · View Herald Transcript
lebedev.ri requested review of this revision.Dec 27 2022, 10:53 AM
gchatelet added inline comments.Dec 27 2022, 11:44 PM
llvm/docs/CommandGuide/llvm-exegesis.rst
213

Maybe add that --skip-codegen implies --skip-measurements as defined later in the code.

llvm/tools/llvm-exegesis/lib/BenchmarkRunner.h
39

[nit] It's probably better to restructure the BenchmarkRunner to only run the benchmark instead of also being responsible for doing codegen.

Also passing multiple consecutive bool arguments seems like a red flag. Using an enum seems more compelling (since one flag implies the other).

lebedev.ri added inline comments.Dec 28 2022, 5:38 AM
llvm/tools/llvm-exegesis/lib/BenchmarkRunner.h
39

[nit] It's probably better to restructure the BenchmarkRunner to only run the benchmark instead of also being responsible for doing codegen.

The separation of BenchmarkRunner::getRunnableConfiguration() and BenchmarkRunner::runConfiguration()
only appeared in refactorings towards parallelization: D140271
It might be doable, but i'm not sure if it makes sense yet.

Also passing multiple consecutive bool arguments seems like a red flag. Using an enum seems more compelling (since one flag implies the other).

Yeah, i've tried that. Let me try once more i guess.

lebedev.ri marked 2 inline comments as done.

@gchatelet thank you for taking a look.
After thinking more, i come up with this approach.

lebedev.ri planned changes to this revision.Dec 28 2022, 7:44 AM

It has occurred to me that this is not what i actually want.
The *full* codegen is slow, but small snippet is fast to codegen,
and it's still useful to be able to to that. So more options..

gchatelet added inline comments.Jan 3 2023, 7:33 AM
llvm/docs/CommandGuide/llvm-exegesis.rst
199–209

I'm confused by this documentation.
Can we list the options and describe what they mean exactly?
What is the difference between skip-codegen and skip-full-codegen?
What are the outputs for the different stages?
Maybe name the options according to what they do instead of what it doesn't do.

llvm/tools/llvm-exegesis/llvm-exegesis.cpp
125–141

It seems better to use more descriptive flags:

  1. Only generate the minimal instruction sequence
  2. Same as 1. but also dumps an excerpt of the sequence (hex encoded)
  3. Same as 2. but also creates the full sequence that can be dumped to a file using --dump-object-to-disk
  4. Same as 3. but also runs the measurement.
lebedev.ri updated this revision to Diff 485995.Jan 3 2023, 7:47 AM
lebedev.ri marked an inline comment as done.

@gchatelet thank you for taking a look!
I acknowledge that the naming seems confusing, but i think it represents the internal flow quite well.

llvm/docs/CommandGuide/llvm-exegesis.rst
199–209

Is this better?
I'm effectively modelling them as stop-before/stop-after of the opt

gchatelet added inline comments.Jan 3 2023, 8:05 AM
llvm/docs/CommandGuide/llvm-exegesis.rst
199–209

This is better but I'd avoid using the skip- part as it's unclear what it's actually doing.
So yes the documentation is improved but changing the flags themselves into more meaningful commands would help as well I think.
How about:

  • prepare-snippet
  • prepare-and-dump-snippet
  • prepare-measured-code
  • measure
lebedev.ri added inline comments.Jan 3 2023, 3:58 PM
llvm/docs/CommandGuide/llvm-exegesis.rst
199–209

I don't see why that is an improvement.

lebedev.ri added inline comments.Jan 3 2023, 4:36 PM
llvm/docs/CommandGuide/llvm-exegesis.rst
199–209

In particular, we are really overloading the "snippet" term here,
and "dump" doesn't really make sense here.
Do we not dump the "measured code"? And we don't dump the actual mini-snippet either.

To be noted, i feel like it's a moot point to argue over the namings. It's mostly irrelevant in the long run.
What do @courbet @RKSimon think? I can change it any way that is agreed.

To be noted, i feel like it's a moot point to argue over the namings. It's mostly irrelevant in the long run.
What do @courbet @RKSimon think? I can change it any way that is agreed.

The naming was a concern for @courbet as well. We discussed this patch together.

To be noted, i feel like it's a moot point to argue over the namings. It's mostly irrelevant in the long run.
What do @courbet @RKSimon think? I can change it any way that is agreed.

I think it's better to express the flag in terms of what you *want* instead of what you *do not want*. You know what you want, but you might not know that you don't want what you don't know exists :)

To be noted, i feel like it's a moot point to argue over the namings. It's mostly irrelevant in the long run.
What do @courbet @RKSimon think? I can change it any way that is agreed.

The naming was a concern for @courbet as well. We discussed this patch together.

Good point. But none of the comments before this one mentioned it in any way.
Let me rename them then.

lebedev.ri updated this revision to Diff 486294.Jan 4 2023, 7:53 AM
lebedev.ri marked 2 inline comments as done.

Ok, how about now?

gchatelet accepted this revision.Jan 5 2023, 4:35 AM
This revision is now accepted and ready to land.Jan 5 2023, 4:35 AM

@gchatelet thank you for the review!

This revision was landed with ongoing or failed builds.Jan 5 2023, 6:47 AM
This revision was automatically updated to reflect the committed changes.
lebedev.ri added inline comments.Jan 5 2023, 8:22 AM
llvm/tools/llvm-exegesis/lib/BenchmarkResult.h
37–42

I just realized, we only changed the user-facing side,
but kept the implementation detail. Is that fine?

gchatelet added inline comments.Jan 6 2023, 5:42 AM
llvm/tools/llvm-exegesis/lib/BenchmarkResult.h
37–42