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
49–50

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
58

what about splitting the if in two subfunctions ?

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

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
66

ASSERT_THAT, since you're taking the front below.

87

ditto

115

ditto

125

should you be testing that ? this is randomized.

157

ditto

174

ditto

192

ditto

193

Same here: any ordering would be OK.

228

ditto

232

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.