[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.

Thank you for the comments.
I fixed the "immeditate" misprint, 
replaced the if statement: if (!(A && B) && !(C && D)) --> if ((!A || !B) && (!C || !D))
and removed the 'FIXME' word in SIFoldOperands.cpp (accordingly to recommendation from Quentin Colombet).


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:178
@@ -177,2 +177,3 @@
 
-    if (!CanCommute || !TII->commuteInstruction(MI))
+    // FIXME: One of operands might be an Imm operand, and OpNo may refer
+    // to it after the call of commuteInstruction() below. Such situations
----------------
qcolombet wrote:
> The comment is just fine without the FIXME in front of it.
> Just remove it, i.e., I do not think there is anything to fix here in the end.
Fixed. I removed the "FIXME" word.

================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:798-799
@@ +797,4 @@
+
+  if (!(OpIdx0 == static_cast<unsigned>(Src0Idx) &&
+        OpIdx1 == static_cast<unsigned>(Src1Idx)) &&
+      !(OpIdx0 == static_cast<unsigned>(Src1Idx) &&
----------------
arsenm wrote:
> I would prefer to deMorgan's law this and distribute the !
Ok, Fixed.

================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:874
@@ -859,2 +873,3 @@
   // FIXME: Workaround TargetInstrInfo::commuteInstruction asserting on
-  // immediate.
+  // immediate. Also, immeditate src0 operand is not handled in
+  // SIInstrInfo::commuteInstruction();
----------------
arsenm wrote:
> Typo: immeditate
Fixed: immeditate -> immediate.

================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:885-887
@@ +884,5 @@
+  if (Src1.isImm()) {
+    // SIInstrInfo::commuteInstruction() does support commuting the immediate
+    // operand src1 in 2 and 3 operand instructions.
+    if (!isVOP2(MI->getOpcode()) && !isVOP3(MI->getOpcode()))
+      return false;
----------------
arsenm wrote:
> It's not entirely accurate to use isVOP2 / isVOP3 for checking the number of operands. VOP* instructions are always available in a VOP3 encoding, but will still have < 3 operands. Checking if AMDGPU::OpName::src2 is a valid operand is a more reliable check. SALU instructions with an immediate can also be commuted, although there is less reason to do so other than canonicalization. Although it looks like isVOP2/isVOP3 is what commuteInstruction already checks so I guess this is OK for now.
Thank you for the explanations.
I am quite happy that you are Ok with the current version of the changes as the fixing of such subtle things should be done by AMDGPU experts. In this change-set I just synchronized the checks in findOpIndicesToCommute() and commuteInstructionImpl() (i.e. re-used the checks from commuteInstructionImpl() in findOpIndicesToCommute()).



http://reviews.llvm.org/D11370