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

Repository
rL LLVM

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
72 ↗(On Diff #150743)

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

tools/llvm-exegesis/lib/MCInstrDescView.h
147 ↗(On Diff #150743)

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 ↗(On Diff #151126)

what about splitting the if in two subfunctions ?

tools/llvm-exegesis/lib/Uops.cpp
92 ↗(On Diff #151126)

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 ↗(On Diff #151127)

ASSERT_THAT, since you're taking the front below.

87 ↗(On Diff #151127)

ditto

115 ↗(On Diff #151127)

ditto

125 ↗(On Diff #151127)

should you be testing that ? this is randomized.

157 ↗(On Diff #151127)

ditto

174 ↗(On Diff #151127)

ditto

192 ↗(On Diff #151127)

ditto

193 ↗(On Diff #151127)

Same here: any ordering would be OK.

228 ↗(On Diff #151127)

ditto

232 ↗(On Diff #151127)

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.