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

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Slava,

I think we are getting close to the final approval :).
Thanks for working on this.

I haven’t looked at the X86 specific part yet, as I would like the patch to be split in two:

- One patch for the change in API and NFC for all the backends.
- One patch to extend the commute code to handle the FMA cases.

Also, a few general comments:

- Some formatting look strange to me, please use clang-format on the patch.
- Please do not insert blank comment lines around comments, i.e.,

// <— Remove those
// <Some comment>
// <— Remove those

- Do not repeat the comment from the header in the cpp files.

Thanks,
-Quentin


================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:299
@@ -254,3 +298,3 @@
   /// converting to different instructions or making non-trivial changes to
   /// commute them, this method can overloaded to do that.
   /// The default implementation simply swaps the commutable operands.
----------------
We can’t override this one anymore, please update the comment.

================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:302
@@ -257,10 +301,3 @@
   /// If NewMI is false, MI is modified in place and returned; otherwise, a
-  /// new machine instruction is created and returned.  Do not call this
-  /// method for a non-commutable instruction, but there may be some cases
-  /// where this method fails and returns null.
-  virtual MachineInstr *commuteInstruction(MachineInstr *MI,
-                                           bool NewMI = false) const;
-
-  /// If specified MI is commutable, return the two operand indices that would
-  /// swap value. Return false if the instruction
-  /// is not in a form which this routine understands.
+  /// new machine instruction is created and returned.
+  ///
----------------
It would be useful to specify what is the behavior when both commute indices are CommuteAnyOperandIndex.

================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:322
@@ +321,3 @@
+  /// method simply returns true if the corresponding operands are commutable
+  /// and returns false otherwise.
+  ///
----------------
Same thing: what happens if both indices are set to CommuteAnyOperandIndex?

================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:124
@@ +123,3 @@
+/// OpIdx2.
+///
+/// If a target has any instructions that are commutable but require converting
----------------
We usually do not repeat the doxygen comment when they are set in the header.
By doing so, we risk to have them being out-of-sync. Therefore, please keep only the one in the header.
(Ditto for the other functions you’ve modified.)

================
Comment at: llvm/lib/CodeGen/TwoAddressInstructionPass.cpp:1167
@@ +1166,3 @@
+/// Returns true if the transformation happened. Otherwise, returns false.
+///
+bool TwoAddressInstructionPass::tryInstructionCommute(MachineInstr *MI,
----------------
Please document the meaning of the other arguments as well, like Dist, BaseOpKilled, etc.

================
Comment at: llvm/lib/CodeGen/TwoAddressInstructionPass.cpp:1182
@@ +1181,3 @@
+    if (OtherOpIdx != BaseOpIdx &&
+        TII->findCommutedOpIndices(MI, BaseOpIdx, OtherOpIdx)) {
+
----------------
Invert the condition and use “continue”. (Per LLVM coding standard.)

================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:179
@@ +178,3 @@
+    // FIXME: OpNo can be commuted with non-reg operand OtherOpNo, but
+    // such test cases are not handled well yet.
+    if (CanCommute &&
----------------
It is not handled at all, right?

Shouldn’t findCommutedOpIndices return only register operands?

================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:888
@@ +887,3 @@
+    // operand src1 in 2 and 3 operand instructions.
+    if (!isVOP2(MI->getOpcode()) && !isVOP3(MI->getOpcode()))
+      return false;
----------------
I am guessing that you shouldn’t be the one doing this can of changes. Or at least, it should be a separate patch.


http://reviews.llvm.org/D11370