[PATCH] D14550: X86-FMA3: Implemented commute transformations for FMA*_Int instructions

[Vyacheslav Klochkov via llvm-commits]

v_klochkov added a comment.

Thank you for the comments and for the approval!
-Slava


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3547
@@ -3468,1 +3546,3 @@
+  // The input opcode does not match with any of the opcodes from the tables.
+  if (FoundOpcodesGroup == nullptr)
     return 0;
----------------
qcolombet wrote:
> Good point!
> Turn that into an assert maybe?
> That way, if it happens, we would know there are things that can be improved :).
> 
> Up to you.
I'll replace it with an assert. Thank you.

================
Comment at: llvm/test/CodeGen/X86/fma-commute-x86.ll:12
@@ +11,3 @@
+; CHECK-NEXT: vmovap{{s|d}} {{\(%rdx\), %xmm0|\(%rcx\), %xmm1}}
+; CHECK-NEXT: vmovap{{s|d}} {{\(%rdx\), %xmm0|\(%rcx\), %xmm1}}
+; CHECK-NEXT: vfmadd213ss %xmm1, %xmm1, %xmm0
----------------
qcolombet wrote:
> I am guessing you put regular expression here because you want the test to be robust against scheduling changes. This is usually not the way we go, i.e., we tend to use the DAG construct for such cases. That being said, I do not remember whether DAG support NEXT. Anyway, for now, we may just match the actual scheduling, unless you saw it changing?
Scheduling may depend on harmless changes. I think I noticed some irregularities in how the parameters are copied to registers (i.e. which param is read first). So, this change was kind of preventive.
Please see my next comment below. It looks like this approach happened to be useful already as it helps to avoid the need in fixing such tests after harmless changes in code-gen.

================
Comment at: llvm/test/CodeGen/X86/fma-intrinsics-x86.ll:13
@@ -12,4 +12,3 @@
 ;
-; CHECK-FMA-WIN-NEXT: vmovaps (%{{(rcx|rdi)}}), %xmm{{0|1}}
-; CHECK-FMA-WIN-NEXT: vmovaps (%{{(rcx|rdx)}}), %xmm{{0|1}}
-; CHECK-FMA-WIN-NEXT: vfmadd213ss     (%r8), %xmm1, %xmm0
+; CHECK-FMA-WIN-NEXT: vmovap{{s|d}} {{\(%rcx\), %xmm0|\(%r8\), %xmm1}}
+; CHECK-FMA-WIN-NEXT: vmovap{{s|d}} {{\(%rcx\), %xmm0|\(%r8\), %xmm1}}
----------------
This test is a good example showing my motivations changing the checks.

There was a misprint at the line 13: "rdi" was occasionally used instead of "rdx".
The test passed with that misprint which means that movaps from rcx was going before movaps from rdx.
After I tried running this test with a harmless patch (hopefully it will be submitted for code-review On Thursday Nov 12), this test failed because movaps from rdx for some unknown reasons started being met first.

I also used "movap{{s|d}}" because in some cases movaps is used to prepare the operand for *SD/*PD instructions, and in some cases movapd is used.


http://reviews.llvm.org/D14550