This patch adds in several functions to ExegesisTarget that will assist
in setting up memory for the planned memory annotations.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Please remember to add context to the diffs with -UU999999 - https://www.llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
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. |
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. |
llvm/tools/llvm-exegesis/lib/X86/Target.cpp | ||
---|---|---|
984 | We discussed a couple solutions or this with Ondrej and Guillaume, for the record:
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. |
@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!
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.
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.
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. |
llvm/tools/llvm-exegesis/lib/X86/Target.cpp | ||
---|---|---|
998–1005 |
No strong opinion, you can keep it as it if you want. |
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.