[PATCH] [x86] add a reassociation optimization to increase ILP via the MachineCombiner pass

[Gerolf Hoflehner]

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6234
@@ +6233,3 @@
+/// reassociation candidate, Commuted will be set to true.
+static MachineInstr *isReassocCandidate(const MachineInstr &Inst,
+                                        unsigned AssocOpcode,
----------------
I'm not happy about the check* parameters that control the pattern match, but I guess it is better to revisit that at a later point when you will support more reassociation pattern. Although I still suggest removing checkPrevOpcode even for this commit.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6286
@@ +6285,3 @@
+/// need to be commuted.
+static MachineCombinerPattern::MC_PATTERN getPattern(bool CommutePrev,
+                                                     bool CommuteRoot) {
----------------
If you like, I think this could be implemented this by by a 2x2 matrix and avoid the conditionals.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6293
@@ +6292,3 @@
+  }
+  // !CommutePrev
+  if (CommuteRoot)
----------------
else? 

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6313
@@ +6312,3 @@
+  bool CommuteRoot;
+  if (MachineInstr *Prev = isReassocCandidate(Root, AssocOpcode, true, true,
+                                              CommuteRoot)) {
----------------
For this commit that parameter could be eliminated. I understand your intention, but it confusing for anyone that comes across the code the first time. So I would add it in a later commit. That will do away with the TODO also.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6334
@@ +6333,3 @@
+/// Attempt the following reassociation to reduce critical path length:
+///   A = (Anything)
+///   B = A op X (Prev)
----------------
Right now the code supports A = ?? op ??. I would make that clear in the comment and change it later in the follow up patch that generalizes this one. Same for the equivalent comments above and below.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6429
@@ +6428,3 @@
+  // C = B op Y (Root)
+  unsigned OpIdx[4][4] = {
+    { 1, 1, 2, 2 },
----------------
How about something like:
Encodes for each pattern where to find operand in Root and Prev. The operand order in the columns is A, B, X, Y.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6436
@@ +6435,3 @@
+
+  reassociateOps(Root, Prev->getOperand(OpIdx[Pattern][0]),
+                 Root.getOperand(OpIdx[Pattern][1]), Root.getOperand(0),
----------------
I think you could make the array static and only pass Root and Prev to reassociateOps(). That would result in a smaller  signature.

http://reviews.llvm.org/D10321

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/