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

[Vyacheslav Klochkov via llvm-commits]

v_klochkov added a comment.

Hi Quentin,

Thank you for the so quick code-review.
I did the refactoring your suggested to do.
Please see the additional changes.

Thank you,
Slava


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

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3502
@@ +3501,3 @@
+    { X86::VFNMSUBSDr132m_Int, X86::VFNMSUBSDr213m_Int, X86::VFNMSUBSDr231m_Int },
+  };
+
----------------
qcolombet wrote:
> 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.
Ok, good idea, thank you. I did the proposed additional changes.

================
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;
----------------
qcolombet wrote:
> This change is not required anymore.
The search loop must find a group of 3 opcodes because this routine is called only after isFMA3() check.
So, technically 'return 0' statement is not reachable now.
I did not remove this check just for additional safety, for example, if someone else adds more opcodes to isFMA3(), but does not add them to the opcode groups defined in this routine. In such scenario it is better to just return 0.


http://reviews.llvm.org/D14550