[PATCH] D11370: Improved the interface of methods commuting operands, improved X86-FMA3 mem-folding&coalescing.

[Elena Demikhovsky]

delena added a subscriber: delena.
delena added a comment.

Hi Slava,

I added some comments, but I'm definitely not a reviewer. I suggest to send a mail to LLVM-dev and tell that you want to improve FMA code and ask people who can review the patch. 
Do you have "OK" for the up-streaming?


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:2931
@@ +2930,3 @@
+///
+/// Do not call this method for a non-commutable instruction.
+/// Even though the instruction is commutable, the method may still
----------------
I suggest to change from "Do not call" to "If you call"

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:2940
@@ -2930,1 +2939,3 @@
+
+
   switch (MI->getOpcode()) {
----------------
Please remove one empty line.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3169
@@ +3168,3 @@
+bool X86InstrInfo::isFMA3(unsigned Opcode) const {
+  switch (Opcode) {
+    case X86::VFMADDSDr132r:     case X86::VFMADDSDr132m:
----------------
Looks huge. I'm not sure but may be these enums in ABC order and we can compare against first and last?
Or auto-generate something?

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3378
@@ +3377,3 @@
+///
+unsigned X86InstrInfo::getFMA3OpcodeToCommuteOperands(MachineInstr *MI,
+                                                      unsigned SrcOpIdx1,
----------------
This method may be static. Right?

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3387
@@ +3386,3 @@
+  //
+  static const struct {
+    int Opc1;
----------------
I suggest to separate scalar from vector. You handle them separately, right?
I also think that you can put all FMA tables in a separate header file.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3546
@@ +3545,3 @@
+///
+bool X86InstrInfo::findCommutedOpIndices(MachineInstr *MI,
+                                         unsigned &SrcOpIdx1,
----------------
I think this interface is inconvenient. I suggest to separate input and output. You can put ~0U as default value of input.


http://reviews.llvm.org/D11370