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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

Some initial comments. I need to look some more.



================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:66
+
+/// Return a register claass equivalent to \p SrcRC, in \p Domain.
+static const TargetRegisterClass *getDstRC(const TargetRegisterClass *SrcRC,
----------------
class*


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:78
+    return &X86::VK64RegClass;
+  assert(false && "add register class");
+  return nullptr;
----------------
Prefer llvm_unreachable over assert(false)


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:117
+
+  const TargetInstrInfo *TII;
+
----------------
Could we just pass TII to the methods that use it? If one of the virtual implementation doesn't need it they can ignore it. That way we don't have to store it in the classes?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:153
+  const TargetInstrInfo *TII;
+  MachineRegisterInfo *MRI;
+
----------------
Same question as above, can we pass TII and MRI to the virtual methods?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:298
+  /// All edges that are included in some closure.
+  static DenseSet<unsigned> EnclosedEdges;
+
----------------
I'm not sure static state in a pass is a good practice. If we ever tried to multithread it would be a problem.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:474
+void Closure::buildClosure(unsigned E) {
+  addToWorklist(E);
+  while (!Worklist.empty()) {
----------------
Should Worklist be a stack variable here that you can pass by reference to addToWorklist? Does it have lifetime beyond building the closure?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:578
+  if (STI->hasBWI()) {
+    createReplacer(X86::MOV8rm, X86::KMOVBkm);
+    createReplacer(X86::MOV32rm, X86::KMOVDkm);
----------------
Isn't KMOVB and all other 'B' instructions part of the DQI feature set?


https://reviews.llvm.org/D37251