[PATCH] D13710: New X86 FMA3*_Int opcodes for scalar FMA intrinsics.

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.

================
Comment at: llvm/test/CodeGen/X86/fma-intrinsics-phi-213-to-231.ll:4
@@ +3,3 @@
+; CHECK-LABEL: fmaddsd_loop:
+; CHECK:   vfmadd213sd        %xmm{{[0-9]+}}, %xmm{{[0-9]+}}, %xmm{{[0-9]+}}
+define <2 x double> @fmaddsd_loop(i32 %iter, <2 x double> %a, <2 x double> %b, <2 x double> %c) {
----------------
v_klochkov wrote:
> qcolombet wrote:
> > Could you add more check lines to check that the registers are set to the right value?
> > This just this check I guess that previously buggy code would also match and we don’t want that.
> The test cases in this LIT test are quite complex, and adding such additional checks should be quite complex and it may
> make the test unstable, pass/fail may depend on many optimizations working on the tested loops. 
> 
> The initial idea of the test fma-intrinsics-phi-213-to-231.ll _probably_ was only
> to check if the corresponding PHI-213-to-231 optimization did its work for FMAs with ADD accumulators.
> The idea of my changes was to check that the optimization 
> DID NOT do changes for SCALAR FMA instructions generated for scalar FMA intrinsics.
> 
> Your guess that the new scalar test case would pass all checks even without my new patch is correct.
> I did an additional investigation and discovered that PHI-213-to-231 optimization never did any changes
> for scalar FMA instructions generated for intrinsics because of the additional REG copies inserted
> in the pattern (see the base version of the file X86InstrFMA.td the lines 194-196).
> 
> After giving it some more thought I think that the scalar test cases I added to this test are
> redundant as PHI-213-to-231 optimization is just a special instruction inserter which is never
> called for the new FMA*_Int instructions.
> 
> The other aspects/transformations like commuting operands should be checked by other specialized tests
> like fma-commutex86.ll added in (D13269).
> 
> Do you agree with my plan to remove the scalar cases from the test?
> Also, it may be useful to add test cases for 128-bit packed FMAs to this test, 
> I just surprisingly realized that the test has only 256-bit test cases in it.
> 
> Do you agree with my plan to remove the scalar cases from the test?

I believe you if you say they are redundant with the one we will add with D13269. So I am fine with that plan.
However, that does not resolve the problem that the check lines for the PHIs test would have still matched the bad code generation.
How do you suggest to fix that?


http://reviews.llvm.org/D13710