[PATCH] D48935: [llvm-exegesis] Provide a way to handle memory instructions.

[Clement Courbet via Phabricator via llvm-commits]

courbet added a comment.

Thanks, PTAL.



================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.h:77
+        : UnalignedPtr(new char[kSize + kAlignment]),
+          AlignedPtr(UnalignedPtr + kAlignment -
+                     (reinterpret_cast<intptr_t>(UnalignedPtr) % kAlignment)) {}
----------------
gchatelet wrote:
> Why not [aligned_alloc](https://en.cppreference.com/w/c/memory/aligned_alloc) ?
> 
the alignment of aligned_alloc has to be an alignment for a valid object. Some X86 instructions require 512 byte alignment, wich is not the alignment of any valid object. 


================
Comment at: tools/llvm-exegesis/lib/Uops.cpp:182
+    Prototype.Snippet.push_back(std::move(II));
+    if (HasMemOperands)
+      instantiateMemoryOperands(Prototype.ScratchSpaceReg, Prototype.Snippet);
----------------
gchatelet wrote:
> This is not ideal because `if (HasMemOperands) instantiateMemoryOperands(...)` is repeated before each return.
> How about a `PrototypeBuilder` struct that encapsulate the memory logic ?
I originally had a lambda called `MaybeInstanciateMemoryOperands`, but TBH I think the repeated `if` makes it clearer.


================
Comment at: unittests/tools/llvm-exegesis/Common/AssemblerUtils.h:75
     llvm::raw_svector_ostream AsmStream(Buffer);
-    assembleToStream(ET, createTargetMachine(), RegsToDef, Instructions,
+    assembleToStream(ET, createTargetMachine(), {}, RegsToDef, Instructions,
                      AsmStream);
----------------
gchatelet wrote:
> Can you introduce a variable for readability?
For the `{}` ? I've added a comment.


================
Comment at: unittests/tools/llvm-exegesis/X86/SnippetGeneratorTest.cpp:242
+  auto Error = Runner.generatePrototype(Opcode).takeError();
+  EXPECT_TRUE((bool)Error);
+  llvm::consumeError(std::move(Error));
----------------
gchatelet wrote:
> Do you need the `(bool)` cast?
Yes, because the macro tries to capture `Error` as an `llvm::Error` and barfs.


Repository:
  rL LLVM

https://reviews.llvm.org/D48935