[PATCH] D27144: [AVX-512] Correctly preserve the passthru semantics of the FMA scalar intrinsics

[Vyacheslav Klochkov via Phabricator via llvm-commits]

v_klochkov added a comment.

Hi Craig,

It looks good to me, I had only 2 minor comments below.

Also, in the process of the review I had the impression that not all needed forms of intrinsics are implemented.
Clearly, that is not your problem, I just wanted to mention it somewhere.
For example, I can see 3 forms {mask, mask3, maskz} for FMADD, but only 1 form {mask3} for FMSUB, etc.

15:35:49 > grep fmsub_sd X86IntrinsicsInfo.h

  X86_INTRINSIC_DATA(avx512_mask3_vfmsub_sd, FMA_OP_SCALAR_MASK3, X86ISD::FMSUB_RND, 0),

15:36:08 > grep fmadd_sd X86IntrinsicsInfo.h

  X86_INTRINSIC_DATA(avx512_mask_vfmadd_sd, FMA_OP_SCALAR_MASK, X86ISD::FMADD_RND, 0),
  X86_INTRINSIC_DATA(avx512_mask3_vfmadd_sd, FMA_OP_SCALAR_MASK3, X86ISD::FMADD_RND, 0),
  X86_INTRINSIC_DATA(avx512_maskz_vfmadd_sd, FMA_OP_SCALAR_MASKZ, X86ISD::FMADD_RND, 0),

Thank you,
Slava



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:31294
 
   // If we're negating a FMA node, then we can adjust the
   // instruction to include the extra negation.
----------------
Can you please change: 'a FMA node' to 'an FMA node'?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:31307
+    case X86ISD::FNMSUB_RND:   NewOpcode = X86ISD::FMADD_RND;    break;
+    // FIXME: We can't handle scalar intrinsic node here because it would only
+    // invert one element and not the whole vector. But we could try to handle
----------------
FNEG negates all elements of the input vector, while fixing the FMA intrinsic would negate only 1 element. Nothing can be done with that here, right? If so, then this comment should be just a regular comment (not a FIXME-comment).
Consider removing the 'FIXME'-word here. Or alternatively you could write here that we could have a special version of FNEG that negates only the lowest element of the FNEG argument.


https://reviews.llvm.org/D27144