This is an archive of the discontinued LLVM Phabricator instance.

[llvm-exegesis] Improve Register Setup.
ClosedPublic

Authored by gchatelet on Sep 10 2018, 5:47 AM.

Diff Detail

Event Timeline

gchatelet created this revision.Sep 10 2018, 5:47 AM
courbet added inline comments.Sep 10 2018, 6:26 AM
tools/llvm-exegesis/lib/Assembler.cpp
173–174

let's keep this code factored out in generateSnippetSetupCode().

tools/llvm-exegesis/lib/BenchmarkRunner.h
46 ↗(On Diff #164660)

you can kill that now.

47 ↗(On Diff #164660)

where are you setting these ?

tools/llvm-exegesis/lib/RegisterValue.h
1 ↗(On Diff #164660)

Can you please split this to a different patch ?

tools/llvm-exegesis/lib/Target.h
38–39

Please comment what happens when the register bit width is not the same as that of the value.

52

As discussed, let's not change this API (+fillMemoryOperands, getMaxMemoryAccessSize). It already handles not supporting memory operands, which is valid for a target.

tools/llvm-exegesis/lib/X86/Target.cpp
104

Conceptually, the "load immediate" opcode should be a function of the size of the register AND value, not just value (e.g. MOV64ri vs MOV64ri32). See my comment below.

121

Is there any case when the size of the value is not the same as that of the register ? If not, let's make this a requirement and crash if the value bit width is not exactly that of the register.

323

let's fail gracefully instead of crashing here. That's not the users' fault.

gchatelet updated this revision to Diff 164671.Sep 10 2018, 7:51 AM
gchatelet marked 5 inline comments as done.
  • Addressing courbet's comments.
gchatelet updated this revision to Diff 164672.Sep 10 2018, 8:00 AM
gchatelet marked 2 inline comments as done.
  • Addressing courbet's comments.
gchatelet updated this revision to Diff 164673.Sep 10 2018, 8:01 AM
  • Addressing courbet's comments.
gchatelet marked an inline comment as done.Sep 10 2018, 8:02 AM
courbet added inline comments.Sep 12 2018, 1:44 AM
tools/llvm-exegesis/lib/Target.cpp
95

this will break exegesis on AARch64. I think we should implement it first.

unittests/tools/llvm-exegesis/AArch64/TargetTest.cpp
44

let's reinstate that.

gchatelet updated this revision to Diff 165770.Sep 17 2018, 8:10 AM
gchatelet marked an inline comment as done.
  • Addressing courbet's comments.
  • Merge conflicts, reinstated AArch64 tests
gchatelet updated this revision to Diff 165777.Sep 17 2018, 9:08 AM
  • Added 'Set' functions for AArch64 GPRs.
courbet added inline comments.Sep 18 2018, 12:09 AM
tools/llvm-exegesis/lib/AArch64/Target.cpp
32

getLoadImmediateOpcode

tools/llvm-exegesis/lib/Assembler.cpp
40

Please keep the comment.

183

You need to keep this.

gchatelet updated this revision to Diff 165908.Sep 18 2018, 2:22 AM
gchatelet marked 4 inline comments as done.
  • Restore tracking of snipped completion.
courbet accepted this revision.Sep 18 2018, 2:41 AM
This revision is now accepted and ready to land.Sep 18 2018, 2:41 AM
This revision was automatically updated to reflect the committed changes.