This is an archive of the discontinued LLVM Phabricator instance.

[llvm-exegesis] Cleaner design without mutable data.
ClosedPublic

Authored by gchatelet on Jun 11 2018, 5:55 AM.

Details

Summary

Previous design was relying on the 'mutate' keyword and was quite confusing. This version separate mutable from immutable data and makes it clearer what changes and what doesn't.

Diff Detail

Event Timeline

gchatelet created this revision.Jun 11 2018, 5:55 AM
gchatelet updated this revision to Diff 150743.Jun 11 2018, 6:51 AM

Removed dead code and fixed documentation.

unit tests ?

tools/llvm-exegesis/lib/Latency.cpp
54–55

let's add a tiny comment there as this triggers my weirdness detector.

tools/llvm-exegesis/lib/MCInstrDescView.h
146–147

selects

gchatelet updated this revision to Diff 151126.Jun 13 2018, 4:55 AM
gchatelet marked 2 inline comments as done.

Adding unit tests for latency and uops.

courbet added inline comments.Jun 13 2018, 5:05 AM
tools/llvm-exegesis/lib/Latency.cpp
62

what about splitting the if in two subfunctions ?

tools/llvm-exegesis/lib/Uops.cpp
92–102

rm this

gchatelet updated this revision to Diff 151129.Jun 13 2018, 5:05 AM

Removing commented function.

courbet added inline comments.Jun 13 2018, 5:10 AM
unittests/tools/llvm-exegesis/X86/SnippetGeneratorTest.cpp
67

ASSERT_THAT, since you're taking the front below.

88

ditto

116

ditto

126

should you be testing that ? this is randomized.

158

ditto

175

ditto

193

ditto

194

Same here: any ordering would be OK.

229

ditto

233

i don;t think you shoul dbe testing the exact assignment

gchatelet updated this revision to Diff 151142.Jun 13 2018, 6:21 AM

Made tests less britle.

gchatelet marked 12 inline comments as done.Jun 13 2018, 6:23 AM
courbet accepted this revision.Jun 13 2018, 6:25 AM
This revision is now accepted and ready to land.Jun 13 2018, 6:25 AM
This revision was automatically updated to reflect the committed changes.