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

[Quentin Colombet]

qcolombet added a comment.

Hi Vyacheslav,

I second Matthias' comments.

Just a few more things, see the inline comments.

Thanks,
-Quentin


================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:100
@@ +99,3 @@
+  /// 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.
----------------
Please define a static constant for this magic value.
I do not like magic values wandering around without context.
I do not have a good naming right now though, maybe UndefinedIndex?

================
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;
+
----------------
MatzeB wrote:
> 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;
I think for the general case instead of having a list of unsigned, we would need a list of pair of unsigned.


http://reviews.llvm.org/D11370