[PATCH] D118301: [Spill2Reg][4/9] Added x86 profitability model.

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:2041
+    llvm_unreachable(
+        "Target didn't implement TargetInstrInfo::isSpill2RegProfitable()");
+  }
----------------
Use the same format as the messages in other callback functions.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:2050
+                             const TargetRegisterInfo *TRI) const {
+    llvm_unreachable("Target does not implement this");
+  }
----------------
same


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:2059
+                                const TargetRegisterInfo *TRI) const {
+    llvm_unreachable("Target does not implement this");
+  }
----------------
same


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:82
+    "spill2reg-mem-instrs", cl::Hidden, cl::init(2),
+    cl::desc("Apply spill2reg if the we find at least this many mem "
+             "instrs within the explored distance."));
----------------
'the'


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:87
+    "spill2reg-vec-instrs", cl::Hidden, cl::init(0),
+    cl::desc("Apply spill-to-reg if we find at most this many vector instrs "
+             "within the explored distance."));
----------------
Be consistent with other description.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:9736
+    if (MO.isReg() && MO.getReg().isPhysical() && HasRC(MO.getReg()) &&
+        TRI->getRegSizeInBits(MO.getReg(), *MRI) >= MinVecBits)
+      return true;
----------------
Instead of checking register size, can you check if MO.getReg() belongs to any vector register class explicitly?



================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:9790
+      BuildMI(*MBB, InsertBeforeIt, DL, InsertMCID, DstReg).addReg(SrcReg);
+  InsertMI->addRegisterKilled(DstReg, TRI);
+  return InsertMI;
----------------
I don't understand this statement. Kill flag means the last use of a register, but DstReg will be used to move the value back to integer register.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118301