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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: tools/llvm-exegesis/lib/Assembler.cpp:92
   MF.push_back(MBB);
+  for (const unsigned Reg : LiveIns) {
+    MBB->addLiveIn(Reg);
----------------
Remove parenthesis.


================
Comment at: tools/llvm-exegesis/lib/Assembler.cpp:171
+
+  for (const unsigned Reg : LiveIns) {
+    MF.getRegInfo().addLiveIn(Reg);
----------------
Remove parenthesis.


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.cpp:141
     }
+    if (Prototype.ScratchSpaceReg) {
+      Configuration.SnippetSetup.LiveIns.push_back(Prototype.ScratchSpaceReg);
----------------
Remove parenthesis.


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.h:77
+        : UnalignedPtr(new char[kSize + kAlignment]),
+          AlignedPtr(UnalignedPtr + kAlignment -
+                     (reinterpret_cast<intptr_t>(UnalignedPtr) % kAlignment)) {}
----------------
Why not [aligned_alloc](https://en.cppreference.com/w/c/memory/aligned_alloc) ?



================
Comment at: tools/llvm-exegesis/lib/MCInstrDescView.h:130
+  // If the prototype uses the provided scratch memory, the register in which
+  // this memory is passed in to the function.
+  unsigned ScratchSpaceReg = 0;
----------------
"the pointer to" this memory is passed


================
Comment at: tools/llvm-exegesis/lib/Target.h:59
+  // size for load/store instructions. This is used to generate independant
+  // memory accesses.
+  virtual unsigned getMaxMemoryAccessSize() const { return 0; }
----------------
I don't understand the comment :-/

How about this :
```
// Returns the maximum number of bytes a load/store instruction can access at once.
// This is typically the size of the largest register available on the processor.
virtual unsigned getLoadStoreMaxSize() const;
```

Also I think there's mismatch between the documentation and the implementation since you say `return a value that is *larger* than ...` but the implementation returns 64.


================
Comment at: tools/llvm-exegesis/lib/Uops.cpp:182
+    Prototype.Snippet.push_back(std::move(II));
+    if (HasMemOperands)
+      instantiateMemoryOperands(Prototype.ScratchSpaceReg, Prototype.Snippet);
----------------
This is not ideal because `if (HasMemOperands) instantiateMemoryOperands(...)` is repeated before each return.
How about a `PrototypeBuilder` struct that encapsulate the memory logic ?


================
Comment at: tools/llvm-exegesis/lib/Uops.cpp:206
         "instruction has tied variables using static renaming.";
+    // Generate
     for (const llvm::MCPhysReg Reg : Op.Tracker->sourceBits().set_bits()) {
----------------
?


================
Comment at: tools/llvm-exegesis/lib/Uops.h:45
+  // instructions until there are this number of instructions.
+  void
+  instantiateMemoryOperands(unsigned ScratchSpaceReg,
----------------
Not sure to understand what you mean by
```
  // If there are less
  // than kMinNumDifferentAddresses in the original snippet, we duplicate
  // instructions until there are this number of instructions.
```


================
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);
----------------
Can you introduce a variable for readability?


================
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));
----------------
Do you need the `(bool)` cast?


Repository:
  rL LLVM

https://reviews.llvm.org/D48935