[PATCH] D11370: Improved the interface of methods commuting operands, improved X86-FMA3 mem-folding&coalescing.
Quentin Colombet
qcolombet at apple.com
Wed Jul 29 10:31:19 PDT 2015
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
More information about the llvm-commits
mailing list