[PATCH] D13269: Improved X86-FMA3 mem-folding & coalescing

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Slava,

Almost there. I think we would benefit from a bit of refactoring.
Tell me if you disagree.

Thanks,
-Quentin


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3327
@@ +3326,3 @@
+  // Define the array that holds FMA opcodes in groups
+  // of 3 opcodes(132, 213, 231) in each group.
+  static const unsigned OpcodeGroups[][3] = {
----------------
Excellent!

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3402
@@ +3401,3 @@
+  const unsigned Form231Index = 2;
+  const unsigned LastFormIndex = 2;
+
----------------
Maybe put three and adapt the comparison.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3405
@@ +3404,3 @@
+  unsigned OpcodeGroupsNum = sizeof(OpcodeGroups) / sizeof(OpcodeGroups[0]);
+  unsigned GroupIndex, FormIndex;
+  for (GroupIndex = 0; GroupIndex < OpcodeGroupsNum; GroupIndex++) {
----------------
Initialize FormIndex to LastFromIndex.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3406
@@ +3405,3 @@
+  unsigned GroupIndex, FormIndex;
+  for (GroupIndex = 0; GroupIndex < OpcodeGroupsNum; GroupIndex++) {
+    for (FormIndex = 0; FormIndex < 3; FormIndex++) {
----------------
Use for (GroupIndex = 0; GroupIndex < OpcodeGroupsNum && LastFormIndex != LastFormIndex; GroupIndex++)

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3407
@@ +3406,3 @@
+  for (GroupIndex = 0; GroupIndex < OpcodeGroupsNum; GroupIndex++) {
+    for (FormIndex = 0; FormIndex < 3; FormIndex++) {
+      if (OpcodeGroups[GroupIndex][FormIndex] == Opc)
----------------
Use < LastFormIndex

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3413
@@ +3412,3 @@
+      // Found the input opcode in the table.
+      break;
+  }
----------------
Remove this block.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3417
@@ +3416,3 @@
+  // Input opcode does not match with any of the opcodes from the table.
+  if (FormIndex > LastFormIndex)
+    return 0;
----------------
Use ==

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3423
@@ +3422,3 @@
+    std::swap(SrcOpIdx1, SrcOpIdx2);
+  }
+
----------------
No need for {}

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3430
@@ +3429,3 @@
+      // (A * b + C)     ==> (A * b + C);
+      // FMA132 A, C, b; ==> FMA231 C, A, b;
+      RetOpc = OpcodeGroups[GroupIndex][Form231Index];
----------------
I like the comments!
Keep them, but I believe we can refactor the code a bit.

What you basically have is given the indices, you map each FormIndex to a new FormIndex and you have three choices each time.
It looks to me like we could do save this table statically like
static const unsigned Mapping[][3] = {
  // SrcOpIdx1 == 1 && SrcOpIdx2 == 2
  // FMA132 A, C, b; ==> FMA231 C, A, b;
  // FMA213 B, A, c; ==> FMA213 A, B, c;
  // FMA231 C, A, b; ==> FMA132 A, C, b;
  { Form231Index,  Form213Index, Form132Index },
  // (SrcOpIdx1 == 1 && SrcOpIdx2 == 3)
  // etc.
  // (SrcOpIdx1 == 2 && SrcOpIdx2 == 3)
};
unsigned Case;
if (SrcOpIdx1 == 1 && SrcOpIdx2 == 2)
  Case = 0;
else  if (SrcOpIdx1 == 1 && SrcOpIdx2 == 3)
  Case = 1;
else if (SrcOpIdx1 == 2 && SrcOpIdx2 == 3)
  Case = 2;
else
  assert(“Invalid commutable indices for FMA”); // or return 0;

RetOpc == Mapping[Case][FormIndex];


http://reviews.llvm.org/D13269