[PATCH] D37251: [X86] Add a pass to convert instruction chains between domains

[Zvi Rackover via Phabricator via llvm-commits]

zvi added inline comments.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:43
+
+static bool isGPR(const TargetRegisterClass *RC) {
+  return RC == &X86::GR8RegClass || RC == &X86::GR16RegClass ||
----------------
Would these functions fit better in X86RegisterInfo?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:49
+static bool isMask(const TargetRegisterClass *RC) {
+  return RC == &X86::VK1RegClass || RC == &X86::VK2RegClass ||
+         RC == &X86::VK4RegClass || RC == &X86::VK8RegClass ||
----------------
Can this check be made cheaper and more maintainable by defining a super class for all K reg class and check for membership in the superclass?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:245
+
+  virtual bool ConvertInstr(MachineInstr *MI) const override { return true; }
+
----------------
How does the delete happen? Are we relying on MachneDCE?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:317
+
+  /// Mark this closure as illegal for reassignment fo all domains.
+  void setAllIllegal() { LegalDstDomains.clear(); }
----------------
*for


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:322
+  void setIllegal(RegDomain RD) {
+    auto I = std::find(LegalDstDomains.begin(), LegalDstDomains.end(), RD);
+    if (I != LegalDstDomains.end())
----------------
STLExtras.h provides a more convenient variant of find:

```
auto I = find(LegalDstDomains, RD);
```


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:332
+  bool isLegal(RegDomain RD) const {
+    return std::find(LegalDstDomains.begin(), LegalDstDomains.end(), RD) !=
+           LegalDstDomains.end();
----------------
return is_contained(LegalDstDomains, RD)


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:374
+
+Closure::Closure(const TargetInstrInfo *TII, MachineRegisterInfo *MRI,
+                 const InstrConverterMap &Converters,
----------------
If the class declaration and definition and in the same unit, make the definition inline in class?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:380
+
+void Closure::addToWorklist(unsigned Reg) {
+  if (EnclosedEdges.count(Reg))
----------------
This methods does more than possibly adding to the worklist. Maybe visitRegister would be a better name?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:403
+  auto I = EnclosedInstrs.find(MI);
+  if (EnclosedInstrs.find(MI) != EnclosedInstrs.end()) {
+    if (I->second != this)
----------------
EnclosedInstrs.find(MI) -> I


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:452
+/// MI.
+static bool usedAsAddr(const MachineInstr &MI, unsigned Reg,
+                       const TargetInstrInfo *TII) {
----------------
Should this be moved out to a common location?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:545
+
+  createReplacerDstCOPY(X86::MOVZX32rm16, X86::KMOVWkm);
+  createReplacerDstCOPY(X86::MOVZX64rm16, X86::KMOVWkm);
----------------
Please add .mir tests for each replacer


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:674
+  for (Closure &C : Closures)
+    if (C.calculateCost(MaskDomain) < 0) {
+      C.Reassign(MaskDomain);
----------------
0.0


================
Comment at: lib/Target/X86/X86TargetMachine.cpp:410
+void X86PassConfig::addMachineSSAOptimization() {
+    addPass(createX86DomainReassignmentPass());
+  TargetPassConfig::addMachineSSAOptimization();
----------------
identation
 


https://reviews.llvm.org/D37251