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

[Guy Blank via Phabricator via llvm-commits]

guyblank marked 28 inline comments as done.
guyblank added a comment.

Thank you all for the comments.



================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:43
+
+static bool isGPR(const TargetRegisterClass *RC) {
+  return RC == &X86::GR8RegClass || RC == &X86::GR16RegClass ||
----------------
zvi wrote:
> Would these functions fit better in X86RegisterInfo?
i'm not sure, there are all the other register classes (GR_ABCD, and friends) which I did not include here. So putting this isGPR function, as is, in X86RegisterInfo would be a bit misleading I think. 


================
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);
----------------
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"



================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:159
+
+  virtual bool isLegal(const MachineInstr *MI) const override { return true; }
+
----------------
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?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:201
+
+    for (auto &MO : MI->operands()) {
+      if (TargetRegisterInfo::isPhysicalRegister(MO.getReg()))
----------------
delena wrote:
> The COPY has only 2 operands, right?
> Why do you need so complex computations here?
I'm not sure I understand why you say these are complex computations, can you suggest a simpler sequence?


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:245
+
+  virtual bool ConvertInstr(MachineInstr *MI) const override { return true; }
+
----------------
zvi wrote:
> How does the delete happen? Are we relying on MachneDCE?
No, this pass will delete the instruction.
ConvertInstr should returns true when the MI needs to be deleted.


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


================
Comment at: lib/Target/X86/X86DomainReassignment.cpp:511
+        setAllIllegal();
+        continue;
+      }
----------------
aaboud wrote:
> Does that make any sense to continue after we clear all Legal Dst Domains? Should not we just return?
I don't think it matters much. 
If we return here, we'll get to the other uses eventually, as part of another closure, and mark them as illegal (since they will try to add a register which is part of another closure).
So we might as well get to them now.




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


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


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


https://reviews.llvm.org/D37251