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

[Amjad Aboud via Phabricator via llvm-commits]

aaboud added inline comments.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:55
+         RC == &X86::VK16WMRegClass || RC == &X86::VK32WMRegClass ||
+         RC == &X86::VK64RegClass || RC == &X86::VK64WMRegClass;
+}
----------------
Please, pick an order and stick to it. "RC == &X86::VK64RegClass" should be moved line 52.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:96
+  /// \returns the cost increment incurred by converting \p MI.
+  virtual double getCostIncr(const MachineInstr *MI) const = 0;
+};
----------------
Would it be better to rename this function as: "getExtraCost" or "getIncreasedCost"?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:102
+public:
+  virtual bool isLegal(const MachineInstr *MI) const override { return true; }
+
----------------
I think you do not need the "virtual" keyword here and in all places below. The "override" keyword is enough.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:124
+    // It's illegal to replace an instruction that implicitly defines a register
+    // with an instruction that doesn't, unless it's dead.
+    for (auto &MO : MI->implicit_operands())
----------------
To make the comment more clear:
"unless it's dead." -> "unless that register is dead."


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:135
+        BuildMI(*MI->getParent(), MI, MI->getDebugLoc(), TII->get(DstOpcode));
+    for (auto &Op : MI->explicit_operands())
+      Bld.add(Op);
----------------
Are we sure we do not need to add any implicit operand?
in the "isLegal" function you allowed implicit operands that are not dead, when the DstOpcode implicitly define that physical register, so in such case we need to add that implicit operand to the new instruction, do not we?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:159
+
+  virtual bool isLegal(const MachineInstr *MI) const override { return true; }
+
----------------
Why do not we need to check that MI opcode and DstOpode have same operand types/semantic?
Otherwise, the loop in line 169 will fail!


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:196
+
+  virtual bool isLegal(const MachineInstr *MI) const override { return true; }
+
----------------
Should not we return false if (MI->getOpcode() != TargetOpcode:COPY)?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:252
+
+typedef std::pair<int, unsigned> InstrConverterKeyTy;
+
----------------
Can you explain this KetTy? what are the "int" and the "unsigned" pair represents?
Please, add a comment.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:305
+          const InstrConverterMap &Converters,
+          const SmallVector<RegDomain, 2> LegalDstDomains);
+
----------------
How about passing the vector by reference? i.e., "&LegalDstDomins".


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:317
+
+  /// Mark this closure as illegal for reassignment fo all domains.
+  void setAllIllegal() { LegalDstDomains.clear(); }
----------------
zvi wrote:
> *for
I think he meant "to all domains".


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:404
+  if (EnclosedInstrs.find(MI) != EnclosedInstrs.end()) {
+    if (I->second != this)
+      setAllIllegal();
----------------
Please, add a comment explaining this case.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:412
+
+  for (RegDomain D : LegalDstDomains) {
+    InstrConverter *IC = Converters.lookup({D, MI->getOpcode()});
----------------
Please, add a comment explaining this behavior.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:436
+
+  for (unsigned E : Edges) {
+    unsigned DestReg =
----------------
I do not understand this code, can you add a comment explaining the logic behind this code?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:511
+        setAllIllegal();
+        continue;
+      }
----------------
Does that make any sense to continue after we clear all Legal Dst Domains? Should not we just return?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:522
+          setAllIllegal();
+          continue;
+        }
----------------
Same question here, why do not we just return!?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:647
+
+  Closure::EnclosedEdges.clear();
+  Closure::EnclosedInstrs.clear();
----------------
If you clear these two containers for each function processing, then why they need to be static?
How about let this function creates them and pass them to all "Closure"s by reference? Can do that at line 665 below.


https://reviews.llvm.org/D37251