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

Repository
rL LLVM

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

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

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

46 ↗(On Diff #164660)

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

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.

119 ↗(On Diff #164660)

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.

318 ↗(On Diff #164660)

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

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

unittests/tools/llvm-exegesis/AArch64/TargetTest.cpp
44 ↗(On Diff #164673)

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

getLoadImmediateOpcode

tools/llvm-exegesis/lib/Assembler.cpp
37 ↗(On Diff #165777)

Please keep the comment.

183 ↗(On Diff #165777)

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.