[PATCH] D14550: X86-FMA3: Implemented commute transformations for FMA*_Int instructions

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Slava,

I haven’t looked at the test cases closely yet, but I think the patch is pretty good.
I am suggesting one refactoring to avoid some code duplication.

When the refactoring is done, I’ll look closer into the test cases.

Thanks for working on this!

-Quentin


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:2976
@@ -2975,3 +2975,3 @@
 /// Otherwise, returns false.
 static bool isFMA3(unsigned Opcode) {
   switch (Opcode) {
----------------
Add an optional parameter: bool *IsIntrinsic = nullptr.
Set it to false at the beginning of the function.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:2994
@@ +2993,3 @@
+    case X86::VFNMSUBSDr132r_Int: case X86::VFNMSUBSDr132m_Int:
+    case X86::VFNMSUBSSr132r_Int: case X86::VFNMSUBSSr132m_Int:
+
----------------
Put all the intrinsic opcodes together to set the boolean to true and fall through to the non intrinsic cases.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3502
@@ +3501,3 @@
+    { X86::VFNMSUBSDr132m_Int, X86::VFNMSUBSDr213m_Int, X86::VFNMSUBSDr231m_Int },
+  };
+
----------------
At first, I was believing the code would remain simpler if we just merge this table with the existing one.
I was guessing we were doing that because we want to know that the opcode is an intrinsic.
Although that is fair, I don’t like the code duplication this implies.

Now, thinking about it, splitting the table is fine, but we can style avoid the code duplication.
Indeed, let say we have a method that tells us before hand that an opcode is an intrinsic.
Using this knowledge, we can:
- Make the search only in the appropriate table.
- Reuse the same code for both tables, this is just a matter of setting the boundaries correctly.

For instance, we could add an optional parameter to isFMA3 that says whether or not the opcode is an intrinsic, then update GroupsNum and some new OpcodeGroups like variable.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3513
@@ -3457,4 +3512,3 @@
   // Look for the input opcode in the OpcodeGroups table.
-  unsigned OpcodeGroupsNum = sizeof(OpcodeGroups) / sizeof(OpcodeGroups[0]);
-  unsigned GroupIndex = 0, FormIndex = FormsNum;
-  for (; GroupIndex < OpcodeGroupsNum && FormIndex == FormsNum; GroupIndex++) {
+  unsigned GroupsNum = sizeof(OpcodeGroups) / sizeof(OpcodeGroups[0]);
+  unsigned GroupIndex = 0;
----------------
Call isFMA3 to get the intrinsic information.
Set a new OpcodeGroups array to either the Intrinsic opcode array or the regular opcode array.
Set GroupsNum accordingly.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3538
@@ -3464,2 +3537,3 @@
     }
   }
+
----------------
We wouldn’t need that loop anymore.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3541
@@ -3468,1 +3540,3 @@
+  // The input opcode does not match with any of the opcodes from the tables.
+  if (FoundOpcodesGroup == nullptr)
     return 0;
----------------
This change is not required anymore.


http://reviews.llvm.org/D14550