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

[Matthias Braun]

MatzeB added a comment.

Some comments on the proposed interfaces:


================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:97-109
@@ -96,1 +96,15 @@
 
+  /// Assigns the (CommutableOpIdx1, CommutableOpIdx2) pair of commutable
+  /// operand indices to (ResultIdx1, ResultIdx2).
+  /// One or both input values of the pair: (ResultIdx1, ResultIdx2) may be
+  /// predefined to some indices or be undefined (designated by ~0U value).
+  /// The predefined result indices cannot be re-defined.
+  /// The function returns true iff after the result pair redefinition
+  /// the fixed result pair is equal to or equivalent to the source pair of
+  /// indices: (CommutableOpIdx1, CommutableOpIdx2). It is assumed here that
+  /// the pairs (x,y) and (y,x) are equivalent.
+  virtual bool fixCommutedOpIndices(unsigned &ResultIdx1,
+                                    unsigned &ResultIdx2,
+                                    unsigned CommutableOpIdx1,
+                                    unsigned CommutableOpIdx2) const;
+
----------------
This is just a helper used to implement some of the other commuting methods. It should not be public and virtual, it doesn't even access any state and could be static or a helper function elsewhere.

================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:274-277
@@ +273,6 @@
+  ///
+  /// The overloaded version of the method with the indices of the
+  /// commuted operands may be used when the commuted instruction has
+  /// more than two operands and thus, there may be preferences in what
+  /// operand must be commuted.
+  ///
----------------
I think just adding Idx1,Idx2 to the existing method should be enough. The two operand commutable case will also work with that and it less confusing than having two overloaded variants around.

================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:289-312
@@ -267,3 +288,26 @@
+
+  /// Returns true iff the routine could find two commutable operands in the
+  /// given machine instruction.
+  /// The 'SrcOpIdx1' and 'SrcOpIdx2' are INPUT and OUTPUT arguments. Their
+  /// input values can be re-defined in this method only if the input values
+  /// are not pre-defined, which is designated by the special value ~0U
+  /// assigned to it.
+  /// If both of indices are pre-defined and refer to some operands, then the
+  /// method simply returns true if the corresponding operands are commutable
+  /// and returns false otherwise.
+  ///
+  /// For example, calling this method this way:
+  ///     unsigned Op1 = 1, Op2 = ~0U;
+  ///     findCommutedOpIndices(MI, Op1, Op2);
+  /// can be interpreted as a query asking to find an operand that would be
+  /// commutable with the operand#1.
+  ///
   virtual bool findCommutedOpIndices(MachineInstr *MI, unsigned &SrcOpIdx1,
                                      unsigned &SrcOpIdx2) const;
 
+  /// Returns true if the specified MI is commutable and the operands with
+  /// indices SrcOpIdx1 and SrcOpIdx2 can swap their values.
+  /// Otherwise, returns false.
+  virtual bool areOpsCommutable(MachineInstr *MI, unsigned SrcOpIdx1,
+                                unsigned SrcOpIdx2) const;
+
----------------
This interface looks complicated, two functions for querying commutability with different "query" styles. I think this should be simplified to a function that simply returns all operands that are commutable. I would imagine something like:

bool findCommutedOpIndicess(MachineInstr *MI, SmallVectorImpl<unsigned> &CommutableOperandNums) const;

or

ArrayRef<unsigned> findCommutedOpIndices(MachineInstr *MI) const;


http://reviews.llvm.org/D11370