[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:135
+        BuildMI(*MI->getParent(), MI, MI->getDebugLoc(), TII->get(DstOpcode));
+    for (auto &Op : MI->explicit_operands())
+      Bld.add(Op);
----------------
guyblank wrote:
> aaboud wrote:
> > 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?
> BuildMI will add the implicit operands from the instruction description, which are the ones allowed to be not dead by "isLegal"
> 
Then please add a comment saying that implicit operands are added/handled by BuildMI, and we only need to add the explicit operands.


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:159
+
+  virtual bool isLegal(const MachineInstr *MI) const override { return true; }
+
----------------
guyblank wrote:
> aaboud wrote:
> > 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!
> I think the responsibility for semantic equivalence is on users of this class (and all other converters).
> 
> But there is a hidden assumption here that the functions will be called only with the MI they were created with, when initializing the converters.
> Does adding the source op code as a member of the converter and checking/asserting on it sound reasonable?
I think it is the best option we have.
Add the source opcode and make sure that isLegal will accept only MI with same source opcode, otherwise it will fail.
you have two options here:
1. actually return false if the MI opcode and the source opcode are not the same.
2. Only assert on that, but always return true. (this is acceptable, if you think that such case is an implementation issue, rather than user input issue).

In addition, you should add an assertion in the ConverInstr(), that the MI is legal:
assert(isLegal(MI) && "Illegal instruction!");


https://reviews.llvm.org/D37251