This is an archive of the discontinued LLVM Phabricator instance.

[llvm-exegesis] Add Target Memory Utility Functions
ClosedPublic

Authored by aidengrossman on May 20 2023, 3:41 AM.

Details

Summary

This patch adds in several functions to ExegesisTarget that will assist
in setting up memory for the planned memory annotations.

Diff Detail

Event Timeline

aidengrossman created this revision.May 20 2023, 3:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2023, 3:41 AM
aidengrossman requested review of this revision.May 20 2023, 3:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2023, 3:41 AM

Format + rebase

courbet added inline comments.May 30 2023, 2:59 AM
llvm/tools/llvm-exegesis/lib/X86/Target.cpp
984

All these are extremely complex. Can't we insert a call to a separate module instead ?

Given that we don't actually care about measuring the setup code, we don't need to inline each function here.

aidengrossman added inline comments.May 30 2023, 5:53 PM
llvm/tools/llvm-exegesis/lib/X86/Target.cpp
984

I agree that they're complex and the readability is not great.

Theoretically, we could do it in a separate module and call into that, but the requirements for setting up a deterministic execution environment with regard to memory require that all the code has to be in blocks of fixed size at known addresses. I don't see any feasible way to do that by just implementing these functions in C/C++ in the llvm-exegesis code base. It's theoretically possible to have an implementation that lives outside of the main llvm-exegesis code base and gets built/linked in to the JITed snippet and emitted to the same block of memory, but decided against that approach since there was still a lot of complexity with that method and it seemed to be a less prominent path.

Very open to suggestions to improve the readability/complexity though since I'm sure I haven't explored the solution space that thoroughly.

Address reviewer feedback (from dependent review), rebase.

courbet added inline comments.Jun 13 2023, 7:37 AM
llvm/tools/llvm-exegesis/lib/X86/Target.cpp
984

We discussed a couple solutions or this with Ondrej and Guillaume, for the record:

  • Writing the code in syscalls in asm or c++ in a separate module and call to that: We agree that this would be very hard to maintain two separate modules.
  • Writing this as textual asm here and using the snippet parsing facilities to transform the result to MCInsts: Still painful because there a a bunch of values that are not constant.

In the end we don;t really have a much better apporach than this oone, but we think that it could be made much more readable by refactoring, e.g. generateSyscall(SYS_munmap, ...) method, roundTo(4096, X86::RSI);

996

should this be using getpagesize() ?

Address reviewer feedback.

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

That would definitely improve readability! Thanks for the suggestion. I've done some refactoring to try and make things more readable based on your suggestions in addition to adding in comments where it seems like things aren't clear. Let me know what you think and if you have any other suggestions.

aidengrossman marked an inline comment as done.Jun 16 2023, 12:53 AM

@courbet This one should be ready for another round of review too when you get a chance. Thanks for all your time and patience so far with reviews!

General comment: all these blindly clobber registers (which is an issue, see my comment in D151025). Should we always save and restore registers ?

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

PageSizeShift ?

998–1005

Another, maybe simpler option is: AND64ri register, -page_size

(BTW thanks for the refactoring, it makes everything much simpler to understand)

courbet added inline comments.Jun 16 2023, 1:45 AM
llvm/tools/llvm-exegesis/lib/X86/Target.cpp
1040

[nit] Use RDI for consistency with the rest of the file

1059

Please create a constant for this.

Right, all these functions do clobber registers (at least currently). However, the only one that actually matters is configurePerfCounter as it is the only one that runs after the registers get initialized to their specified values. I have a TODO to fix this.

Right, all these functions do clobber registers (at least currently).

Let's document this then :)

However, the only one that actually matters is configurePerfCounter as it is the only one that runs after the registers get initialized to their specified values. I have a TODO to fix this.

SG, let's do it now given that as such the code won't work as is.

aidengrossman marked 3 inline comments as done.

Address reviewer feedback

Just added in register saving to configurePerfCounter() and noted that the functions may clobber registers in Target.h. Probably should've just done it to begin with. New changes should be ready for review.

llvm/tools/llvm-exegesis/lib/X86/Target.cpp
998–1005

I find the shift left/right pattern to be a little bit more intuitive since using an AND instruction involves understanding a bitwise "trick" when reading, but it definitely is shorter. I don't have any particularly strong feelings on this though, so if you want me to change it, just let me know.

courbet accepted this revision.Jun 16 2023, 3:57 AM
courbet added inline comments.
llvm/tools/llvm-exegesis/lib/X86/Target.cpp
998–1005

I find the shift left/right pattern to be a little bit more intuitive since using an AND instruction involves understanding a bitwise "trick" when reading, but it definitely is shorter. I don't have any particularly strong feelings on this though, so if you want me to change it, just let me know.

No strong opinion, you can keep it as it if you want.

This revision is now accepted and ready to land.Jun 16 2023, 3:57 AM
This revision was landed with ongoing or failed builds.Jun 26 2023, 12:25 PM
This revision was automatically updated to reflect the committed changes.