This is an archive of the discontinued LLVM Phabricator instance.

Update llvm-exegesis to cover latency through another opcode.
ClosedPublic

Authored by gchatelet on May 14 2018, 2:28 AM.

Details

Reviewers
courbet
gchatelet
Summary

Restructuring the code to measure latency and uops.
The end goal is to have this program spawn another process to deal with SIGILL and other malformed programs. It is not yet the case in this redesign, it is still the main program that runs the code (and may crash).
It now uses BitVector instead of Graph for performance reasons.

Diff Detail

Event Timeline

gchatelet created this revision.May 14 2018, 2:28 AM
courbet added inline comments.May 14 2018, 5:26 AM
tools/llvm-exegesis/lib/Assembler.cpp
17
tools/llvm-exegesis/lib/Assembler.h
44

doc ?

58

doc ?

62

doc ?

64

doc ?

courbet added inline comments.May 14 2018, 5:54 AM
tools/llvm-exegesis/lib/Assembler.cpp
205

Given that you're moving from AC, I don't think it makes sense to have AC be a member variable here. AC should not be accessible outside of this function.

231

it'd be better if the error was handled by the caller. BTW I don't see who's calling that.

tools/llvm-exegesis/lib/BenchmarkResult.cpp
60–64

Please rebase.

92

what about letting the caller handle the error ?

tools/llvm-exegesis/lib/BenchmarkRunner.cpp
32

static + remove namespace

33

It does not really make sense to create a function to get the reserved registers. Could you refactor this ?

84–87

let's avoid creating a target machine several times. It could be a State member.

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

why not private ?

60

Let's keep llvm::Expected<std::vector<llvm::MCInst>> if you don't mind.

61

ditto

63–69

ditto

tools/llvm-exegesis/lib/Latency.cpp
22

static

60

let's take the OS as a parameter. This will allow disabling debug or dumping it to a field of the benchmark results.

tools/llvm-exegesis/lib/MCInstrDescView.h
2

header + file doc

gchatelet planned changes to this revision.May 14 2018, 6:15 AM
gchatelet marked 6 inline comments as done.
gchatelet added inline comments.
tools/llvm-exegesis/lib/Assembler.cpp
205

Yes, that's part of the redesign I intend to do. I added some FIXME in the header file.

231

Nobody does for now, it's a leftover from the code I wrote earlier to deal with the process spawning. I'll just remove it for now.

gchatelet updated this revision to Diff 146588.May 14 2018, 6:26 AM
gchatelet marked an inline comment as done.
  • Add documentation to Assembler.h.
gchatelet updated this revision to Diff 146590.May 14 2018, 6:30 AM
  • Fix missing static in Assembler.cpp.
gchatelet updated this revision to Diff 146593.May 14 2018, 6:36 AM
gchatelet marked 4 inline comments as done.
  • Update documentation in BenchmarkResult.
gchatelet planned changes to this revision.May 14 2018, 6:37 AM
gchatelet added inline comments.
tools/llvm-exegesis/lib/BenchmarkResult.cpp
92

Let's do this in a separate Patch if you don't mind.

gchatelet updated this revision to Diff 146596.May 14 2018, 6:55 AM
gchatelet marked 5 inline comments as done.
  • Return an llvm::Expected instead of passing vector by ref.
gchatelet planned changes to this revision.May 14 2018, 6:59 AM
gchatelet marked 3 inline comments as done.
gchatelet added inline comments.
tools/llvm-exegesis/lib/BenchmarkRunner.cpp
33

It's part of the redesign of Assembler.{h/cpp}. I'll add a FIXME.

gchatelet updated this revision to Diff 146600.May 14 2018, 7:04 AM
gchatelet marked 3 inline comments as done.
  • Add documentation to Assembler.h.
  • Fix missing static in Assembler.cpp.
  • Update documentation in BenchmarkResult.
  • Return an llvm::Expected instead of passing vector by ref.
  • Removed private namespace and added FIXME.
gchatelet updated this revision to Diff 146601.May 14 2018, 7:12 AM
  • Added Description to MCInstrDescView.h.
gchatelet updated this revision to Diff 146603.May 14 2018, 7:21 AM
gchatelet marked an inline comment as done.
  • Added Fixme to BenchmarkRunner.
  • Make create snippet debug information an argument of the function.
gchatelet planned changes to this revision.May 14 2018, 7:21 AM
gchatelet added inline comments.
tools/llvm-exegesis/lib/BenchmarkRunner.cpp
84–87

Unfortunately the target machine is consumed by the ExecutableFunction to create the ExecutionEngine.
I'm not sure if I can reuse the ExecutionEngine, I'll try this in a following Patch to address the FIXMEs.

gchatelet requested review of this revision.May 14 2018, 7:22 AM
gchatelet marked 3 inline comments as done.
courbet added inline comments.May 14 2018, 8:16 AM
tools/llvm-exegesis/lib/AliasingTracker.cpp
2

Missing header. please check all other files.

tools/llvm-exegesis/lib/Assembler.cpp
1–2

file header.

205

My point is: where else is AC used ? If it's used outside of the ctor, it's probably wrong, because AC was moved from. If it's use donly in the ctor, why not make it local to the ctor now ?

tools/llvm-exegesis/lib/Assembler.h
67

s/TM/Instructions ?

69

"it is assembled (for TM) to AsmStream,..."

75

"Creates an ObjectFile in the format understood by TM"

tools/llvm-exegesis/lib/BenchmarkResult.h
46–48

Did you clang-format your patch ?

tools/llvm-exegesis/lib/BenchmarkRunner.cpp
36

let's inline getReservedRegs() here.

courbet added inline comments.May 15 2018, 12:45 AM
tools/llvm-exegesis/lib/AliasingTracker.h
42

RegisterAliasingTracker because Aliasing has too many meanings.

tools/llvm-exegesis/lib/MCInstrDescView.cpp
153

[style]randomGenerator

155

let's seed with a flag or a constexpr (unittests will thank you).

161

[style] static

161

[style]randomIndex

163

[style]Dis

168

[style] static+randomElement

180

Add a FIXME to explore immediate values too.

206

[style] randomBit

tools/llvm-exegesis/lib/MCInstrDescView.h
80

RegisterOperandAssignment ?

85

Reg

92
// There are two reasons why operands would alias:
// - The registers assigned to each of the operands are the same or alias each other (e.g. AX/AL)
// - The operands are tied.
116

Let's get rid of SmallSet and use std::find

120

ideally this would come from the main (or tests). Possibly in a subsequent revision.

128
AliasingConfigurations AliasingConfigurations::createRandomAssignment(const AliasingConfigurations &AliasingConfigurations);
tools/llvm-exegesis/lib/PerfHelper.cpp
113

I submitted this independently for you as r332330, please rebase.

tools/llvm-exegesis/lib/Uops.cpp
83–86

static

courbet added inline comments.May 15 2018, 1:30 AM
tools/llvm-exegesis/lib/MCInstrDescView.h
131

clearVariableAssignments ?

tools/llvm-exegesis/lib/Uops.cpp
112

hasTiedOperands

125

getTiedVariables

133

remove

174

remove assert, add an if, say it in debug and return an error.

232

format

gchatelet updated this revision to Diff 146764.May 15 2018, 2:35 AM
  • Add documentation to Assembler.h.
  • Fix missing static in Assembler.cpp.
  • Update documentation in BenchmarkResult.
  • Return an llvm::Expected instead of passing vector by ref.
  • Removed private namespace and added FIXME.
  • Remove private namespace.
  • Added Description to MCInstrDescView.h.
  • Added Fixme to BenchmarkRunner.
  • Make create snippet debug information an argument of the function.
  • Rename AliasingTracker files into RegisterAliasing.
gchatelet updated this revision to Diff 146787.May 15 2018, 4:55 AM
gchatelet marked 4 inline comments as done.
  • Add documentation to Assembler.h.
  • Fix missing static in Assembler.cpp.
  • Update documentation in BenchmarkResult.
  • Return an llvm::Expected instead of passing vector by ref.
  • Removed private namespace and added FIXME.
  • Remove private namespace.
  • Added Description to MCInstrDescView.h.
  • Added Fixme to BenchmarkRunner.
  • Make create snippet debug information an argument of the function.
  • Rename AliasingTracker files into RegisterAliasing.
  • Add header in Assembler.cpp.
  • Fix comments in Assembler.h.
  • Simplify Assembler file and remove most of the clutter.
gchatelet updated this revision to Diff 146788.May 15 2018, 4:57 AM
gchatelet marked an inline comment as done.
  • Update comments and fix missing line feed.
gchatelet planned changes to this revision.May 15 2018, 4:58 AM
gchatelet marked an inline comment as done.
gchatelet updated this revision to Diff 146792.May 15 2018, 5:14 AM
gchatelet marked 6 inline comments as done.
  • Get rid of SmallSet.
gchatelet updated this revision to Diff 146794.May 15 2018, 5:19 AM
  • rename clearVariable in clearVariableAssignments.
gchatelet updated this revision to Diff 146795.May 15 2018, 5:25 AM
gchatelet marked 3 inline comments as done.
  • Fix style for random generator functions.
gchatelet updated this revision to Diff 146799.May 15 2018, 5:55 AM
gchatelet marked 11 inline comments as done.
  • Add documentation to Assembler.h.
  • Fix missing static in Assembler.cpp.
  • Update documentation in BenchmarkResult.
  • Return an llvm::Expected instead of passing vector by ref.
  • Removed private namespace and added FIXME.
  • Remove private namespace.
  • Added Description to MCInstrDescView.h.
  • Added Fixme to BenchmarkRunner.
  • Make create snippet debug information an argument of the function.
  • Rename AliasingTracker files into RegisterAliasing.
  • Add header in Assembler.cpp.
  • Fix comments in Assembler.h.
  • Simplify Assembler file and remove most of the clutter.
  • Update comments and fix missing line feed.
  • Get rid of SmallSet.
  • rename clearVariable in clearVariableAssignments.
  • Fix style for random generator functions.
  • Add a FIXME to explore immediate values.
  • Fix private namespace and code style.
  • Fix wrong rebase
  • Turn an assert into a real error.
gchatelet planned changes to this revision.May 15 2018, 5:55 AM
gchatelet added inline comments.
tools/llvm-exegesis/lib/MCInstrDescView.h
120

Yes let's do this in a subsequent version.

gchatelet updated this revision to Diff 146801.May 15 2018, 6:18 AM
gchatelet marked 2 inline comments as done.
  • Add documentation to Assembler.h.
  • Fix missing static in Assembler.cpp.
  • Update documentation in BenchmarkResult.
  • Return an llvm::Expected instead of passing vector by ref.
  • Removed private namespace and added FIXME.
  • Remove private namespace.
  • Added Description to MCInstrDescView.h.
  • Added Fixme to BenchmarkRunner.
  • Make create snippet debug information an argument of the function.
  • Rename AliasingTracker files into RegisterAliasing.
  • Add header in Assembler.cpp.
  • Fix comments in Assembler.h.
  • Simplify Assembler file and remove most of the clutter.
  • Update comments and fix missing line feed.
  • Get rid of SmallSet.
  • rename clearVariable in clearVariableAssignments.
  • Fix style for random generator functions.
  • Add a FIXME to explore immediate values.
  • Fix private namespace and code style.
  • Fix wrong rebase
  • Turn an assert into a real error.
  • Added FIXME.
tools/llvm-exegesis/lib/MCInstrDescView.cpp
155

Added a FIXME, I'll do this in a follow up patch.

tools/llvm-exegesis/lib/MCInstrDescView.h
128

Added a FIXME, I'll do it in a follow up patch.

gchatelet updated this revision to Diff 146833.May 15 2018, 8:17 AM
  • Adding informations inside the YAML output (code snippet, strategy).

Only minor comments left.

tools/llvm-exegesis/lib/Assembler.h
49

s/TM/the host/ (there's no TM anymore).

tools/llvm-exegesis/lib/MCInstrDescView.cpp
2

file header.

12

static

97

static

tools/llvm-exegesis/lib/RegisterAliasing.cpp
1 ↗(On Diff #146833)

file header.

tools/llvm-exegesis/lib/RegisterAliasing.h
41 ↗(On Diff #146833)

Let's rename this to "RegisterAliasingTracker"

75 ↗(On Diff #146833)

ditto

gchatelet updated this revision to Diff 147043.May 16 2018, 4:00 AM
gchatelet marked 10 inline comments as done.
  • Add documentation to Assembler.h.
  • Fix missing static in Assembler.cpp.
  • Update documentation in BenchmarkResult.
  • Return an llvm::Expected instead of passing vector by ref.
  • Removed private namespace and added FIXME.
  • Remove private namespace.
  • Added Description to MCInstrDescView.h.
  • Added Fixme to BenchmarkRunner.
  • Make create snippet debug information an argument of the function.
  • Rename AliasingTracker files into RegisterAliasing.
  • Add header in Assembler.cpp.
  • Fix comments in Assembler.h.
  • Simplify Assembler file and remove most of the clutter.
  • Update comments and fix missing line feed.
  • Get rid of SmallSet.
  • rename clearVariable in clearVariableAssignments.
  • Fix style for random generator functions.
  • Add a FIXME to explore immediate values.
  • Fix private namespace and code style.
  • Fix wrong rebase
  • Turn an assert into a real error.
  • Added FIXME.
  • Adding informations inside the YAML output (code snippet, strategy).
  • Fix typo in documentation.
  • Fix missing header and code style.
  • Rename AliasingTracker and AliasingTrackerCache.
gchatelet accepted this revision.May 16 2018, 4:01 AM
This revision is now accepted and ready to land.May 16 2018, 4:01 AM
courbet accepted this revision.May 16 2018, 4:07 AM
courbet requested changes to this revision.May 16 2018, 4:44 AM
courbet added inline comments.
tools/llvm-exegesis/lib/MCInstrDescView.h
63

this breaks the build in -Wall mode. Use Var.

83

ditto

This revision now requires changes to proceed.May 16 2018, 4:44 AM
gchatelet updated this revision to Diff 147056.May 16 2018, 4:58 AM
gchatelet marked 2 inline comments as done.
  • Add documentation to Assembler.h.
  • Fix missing static in Assembler.cpp.
  • Update documentation in BenchmarkResult.
  • Return an llvm::Expected instead of passing vector by ref.
  • Removed private namespace and added FIXME.
  • Remove private namespace.
  • Added Description to MCInstrDescView.h.
  • Added Fixme to BenchmarkRunner.
  • Make create snippet debug information an argument of the function.
  • Rename AliasingTracker files into RegisterAliasing.
  • Add header in Assembler.cpp.
  • Fix comments in Assembler.h.
  • Simplify Assembler file and remove most of the clutter.
  • Update comments and fix missing line feed.
  • Get rid of SmallSet.
  • rename clearVariable in clearVariableAssignments.
  • Fix style for random generator functions.
  • Add a FIXME to explore immediate values.
  • Fix private namespace and code style.
  • Fix wrong rebase
  • Turn an assert into a real error.
  • Added FIXME.
  • Adding informations inside the YAML output (code snippet, strategy).
  • Fix typo in documentation.
  • Fix missing header and code style.
  • Rename AliasingTracker and AliasingTrackerCache.
  • Rename Variable to Var.
  • Rename Operand variable into Op.
courbet accepted this revision.May 16 2018, 11:17 PM
This revision is now accepted and ready to land.May 16 2018, 11:17 PM
courbet closed this revision.May 22 2018, 11:24 PM
unittests/tools/llvm-exegesis/X86/InMemoryAssemblerTest.cpp