[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/Uops.cpp:182
+    Prototype.Snippet.push_back(std::move(II));
+    if (HasMemOperands)
+      instantiateMemoryOperands(Prototype.ScratchSpaceReg, Prototype.Snippet);
----------------
courbet wrote:
> 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.
> I originally had a lambda called MaybeInstanciateMemoryOperands, but TBH I think the repeated if makes it clearer.

The `if` statement might be clearer than the Lambda but it is as error prone (you can omit the lambda and it compiles but it's broken).
My point was, if you use a Builder it is correct by design (and it removes visual clutter), true that it adds a struct to the file though.


================
Comment at: tools/llvm-exegesis/lib/Uops.h:45
+  // instructions until there are this number of instructions.
+  // For example, assuming kMinNumDifferentAddresses=5, if the original snippet
+  // is:
----------------
"assuming kMinNumDifferentAddresses=5 **and getMaxMemoryAccessSize()=64**"


Repository:
  rL LLVM

https://reviews.llvm.org/D48935