[PATCH] D51856: [llvm-exegesis] Improve Register Setup.

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: tools/llvm-exegesis/lib/Assembler.cpp:173
 
-  bool IsSnippetSetupComplete = false;
-  std::vector<llvm::MCInst> SnippetWithSetup =
-      generateSnippetSetupCode(RegsToDef, ET, *TM, IsSnippetSetupComplete);
-  if (!SnippetWithSetup.empty()) {
-    SnippetWithSetup.insert(SnippetWithSetup.end(), Instructions.begin(),
-                            Instructions.end());
-    Instructions = SnippetWithSetup;
+  std::vector<llvm::MCInst> Code;
+  const llvm::MCSubtargetInfo *const MSI = TM->getMCSubtargetInfo();
----------------
let's keep this code factored out in generateSnippetSetupCode().


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.h:46
   // registers initial values.
   std::vector<unsigned> RegsToDef;
+  std::vector<RegisterValue> RegisterInitialValues;
----------------
you can kill that now.


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.h:47
   std::vector<unsigned> RegsToDef;
+  std::vector<RegisterValue> RegisterInitialValues;
 
----------------
where are you setting these ?


================
Comment at: tools/llvm-exegesis/lib/RegisterValue.h:1
+//===-- RegisterValue.h -----------------------------------------*- C++ -*-===//
+//
----------------
Can you please split this to a different patch ?


================
Comment at: tools/llvm-exegesis/lib/Target.h:38
 
   // Generates code to move a constant into a the given register.
   virtual std::vector<llvm::MCInst>
----------------
Please comment what happens when the register bit width is not the same as that of the value.


================
Comment at: tools/llvm-exegesis/lib/Target.h:46
+  // default calling convention.
+  virtual unsigned getScratchMemoryRegister(const llvm::Triple &) const = 0;
 
----------------
As discussed, let's not change this API (+fillMemoryOperands, getMaxMemoryAccessSize). It already handles not supporting memory operands, which is valid for a target.


================
Comment at: tools/llvm-exegesis/lib/X86/Target.cpp:102
 
+static unsigned GetLoadImmediateOpcode(const llvm::APInt &Value) {
+  switch (Value.getBitWidth()) {
----------------
Conceptually, the "load immediate" opcode should be a function of the size of the register AND value, not just value (e.g. MOV64ri vs MOV64ri32). See my comment below.


================
Comment at: tools/llvm-exegesis/lib/X86/Target.cpp:119
+                                  unsigned MaxBitWidth) {
+  assert(Value.getBitWidth() <= MaxBitWidth && "Value too big to fit register");
+  return llvm::MCInstBuilder(GetLoadImmediateOpcode(Value))
----------------
Is there any case when the size of the value is not the same as that of the register ? If not, let's make this a requirement and crash if the value bit width is not exactly that of the register.


================
Comment at: tools/llvm-exegesis/lib/X86/Target.cpp:318
+      return CI.popFlagAndFinalize();
+    llvm_unreachable("Not yet implemented");
   }
----------------
let's fail gracefully instead of crashing here. That's not the users' fault.


Repository:
  rL LLVM

https://reviews.llvm.org/D51856