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

[Vyacheslav Klochkov via llvm-commits]

v_klochkov added a comment.

The code 'if (MI->IsCommutable() && TII->commuteInstruction(MI))' works differently before and after the changes in commuteInstruction() method .

BEFORE:

  If MI has the attribute 'MCID::Commutable' set to true, then 
      1) go to AMDGPU specific implementation of commuteInstruction()
          and commute operands (even if those are Reg and Imm).

AFTER:

  If MI has the attribute 'MCID::Commutable' set to true, then 
    1) go to TargetInstrInfo::commuteInstruction() and try to commute operands
        1a) call AMDGPU specific implementation of findCommutedOpIndices()
        1b) if could find commutable operands, then call AMDGPU specific implementation of commuteInstruction()

The step 1a) returned false and no operands commute happened, which caused LIT tests fails.
So, I just synchronized understanding of commutable operands in AMDGPU methods findCommutedOpIndices() and commuteInstruction(). 
The updated findCommutedOpIndices() returns true for Reg and Imm operands if such can be commuted by commuteInstruction().

The fix in SIFoldOperands.cpp was needed because the updated findCommutedOpIndices() may return true for commutable Reg and Imm operands. 
In such cases the index of Imm operand is stored into 'FoldList' std::vector object. 
Later (downstream of the optimization) the elements of returned FoldList are treated as if they are all REG operands, which causes assert violations for Imm operands stored to 'FoldList'.

I'll fix the mentioned formatting issues, and will try to run format-clang tools.

Thanks,
Slava


================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:287-288
@@ -251,6 +286,4 @@
   }
 
-  /// If a target has any instructions that are commutable but require
-  /// 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.
+  // This constant can be used as an input value of operand index passed to
+  // the method findCommutedOpIndices() to tell the method that the
----------------
Fixed.

================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:304-331
@@ -257,11 +303,30 @@
   /// 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.
+  ///
+  /// Do not call this method for a non-commutable instruction or
+  /// for non-commuable operands.
+  /// Even though the instruction is commutable, the method may still
+  /// fail to commute the operands, null pointer is returned in such cases.
+  MachineInstr *commuteInstruction(MachineInstr *MI,
+                                   bool NewMI = false,
+                                   unsigned OpIdx1 = CommuteAnyOperandIndex,
+                                   unsigned OpIdx2 = CommuteAnyOperandIndex) const;
+
+  /// Returns true iff the routine could find two commutable operands in the
+  /// given machine instruction.
+  /// The 'SrcOpIdx1' and 'SrcOpIdx2' are INPUT and OUTPUT arguments.
+  /// If any of the INPUT values is set to the special value
+  /// 'CommuteAnyOperandIndex' then the method arbitrarily picks a commutable
+  /// operand, then returns its index in the corresponding argument.
+  /// If both of INPUT values are set to 'CommuteAnyOperandIndex' then method
+  /// looks for 2 commutable operands.
+  /// If INPUT values refer to some operands of MI, 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 = CommuteAnyOperandIndex;
+  ///     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,
----------------
Fixed.

================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:124
@@ -125,1 +123,3 @@
+                                                      unsigned Idx1,
+                                                      unsigned Idx2) const {
   const MCInstrDesc &MCID = MI->getDesc();
----------------
Ok, removed the duplicated comments/descriptions.

================
Comment at: llvm/lib/CodeGen/TwoAddressInstructionPass.cpp:1167
@@ +1166,3 @@
+/// 
+/// 'DstOpIdx' specifies the index of MI def operand.
+/// 'BaseOpKilled' specifies if the register associated with 'BaseOpIdx'
----------------
Fixed.

================
Comment at: llvm/lib/CodeGen/TwoAddressInstructionPass.cpp:1182
@@ +1181,3 @@
+  unsigned DstOpReg = MI->getOperand(DstOpIdx).getReg();
+  unsigned BaseOpReg = MI->getOperand(BaseOpIdx).getReg();
+  for (; OtherOpIdx < MI->getDesc().getNumOperands(); OtherOpIdx++) {
----------------
Fixed.

================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:179
@@ +178,3 @@
+    // FIXME: One of operands might be an Imm operand, and OpNo may refer
+    // to it after the call of commuteInstruction() below. Such situations
+    // are avoided here explicitly as OpNo must be a register operand to be
----------------
findCommutedOpIndices() returns commutable operands. They can be Imm operands for AMDGPU target.
I also updated the FIXME comment to make it more informative.


http://reviews.llvm.org/D11370