[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 still do not get the AMDGPU changes.

> Unfortunately, I had to do those changes. 

>  Also, those changes cannot be separated from the interface changes I did for

>  commuteInstruction() and findCommutedOpIndices().


We didn't change the basic semantic of those APIs, just making them more powerful, right?
So, why do we need to change the way we use them for this target.
Indeed, " if (MI->IsCommutable() && TII->commuteInstruction(MI)) {}" seems like a reasonable pattern to me.

What am I missing?

Thanks,
-Quentin


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:212
@@ +211,3 @@
+  }
+  else if (ResultIdx1 == CommuteAnyOperandIndex) {
+    if (ResultIdx2 == CommutableOpIdx1)
----------------
The formatting still looks suspicious to me.
I expect the "else" to be on the same line as the '}'.
Have you run clang-format?

================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:186
@@ -179,1 +185,3 @@
+
+    if (!CanCommute || !TII->commuteInstruction(MI, false, CommuteIdx0, CommuteIdx1))
       return false;
----------------
Since you said the target can commute imm and reg operand, why do we need this change?

================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:796
@@ -786,2 +795,3 @@
                                            AMDGPU::OpName::src1);
-  if (Src1Idx == -1)
+  assert(Src1Idx != -1 && "Should always have src1 operand");
+
----------------
I still don't get why we need to turn this if into an assert.


http://reviews.llvm.org/D11370