[PATCH] D151023: [llvm-exegesis] Add Target Memory Utility Functions

[Aiden Grossman via Phabricator via llvm-commits]

aidengrossman added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/Target.cpp:978
 
+#ifdef __linux__
+std::vector<MCInst> ExegesisX86Target::generateLowerMunmap() const {
----------------
courbet wrote:
> aidengrossman wrote:
> > courbet wrote:
> > > 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.
> > 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.
> 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 `MCInst`s: 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);`
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151023/new/

https://reviews.llvm.org/D151023