[PATCH] [AVX-512] - Add FMA instruction with Rounding mode

[Adam Nemet]

> On Jan 6, 2015, at 11:12 PM, Demikhovsky, Elena <elena.demikhovsky at intel.com> wrote:
> 
> 
> 
> -  Elena
> 
> -----Original Message-----
> From: Adam Nemet [mailto:anemet at apple.com <mailto:anemet at apple.com>] 
> Sent: Wednesday, January 07, 2015 03:04
> To: Badouh, Asaf; Demikhovsky, Elena; anemet at apple.com <mailto:anemet at apple.com>
> Cc: cameron.mcinally at nyu.edu <mailto:cameron.mcinally at nyu.edu>; llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> Subject: Re: [PATCH] [AVX-512] - Add FMA instruction with Rounding mode
> 
> REPOSITORY
>  rL LLVM
> 
> ================
> Comment at: lib/Target/X86/X86ISelLowering.h:374
> @@ -373,1 +373,3 @@
> 
> +      ROUNDMODE,
> +
> ----------------
> Hmm, has this been completely thought through?  I may have missed the discussion... 
> 
> Looks like you're introducing a new node that if it surrounds an FMA op it changes its rounding mode?
> 
> What happens if the two nodes get separated by some transformation?
> [Demikhovsky, Elena] We surround with VSELECT to set mask for intrinsic. The idea here is the same, even less  risky. The stage is late, all transformations are behind.
> We plan to use this node for all FP arithmetic and conversion intrinsics.

Well, VSELECT can be translated to an instruction, so I think that’s different.  If the VSELECT and the op get separated we generate less efficient code so that’s always OK.  In this case I am not convinced that we can’t generate the wrong result.

My main point is that unless this was discussed somewhere else, it shouldn’t be hidden in a patch like this.  It’s an important enough design decision for the AVX512 SDAG IR to warrant some scrutiny.

I suggest to properly comment this node and provide explanation in the review itself and CC a few more people (Nadav, Chandler, Hal, Jim, etc.)

Thanks,
Adam

> 
> ================
> Comment at: lib/Target/X86/X86InstrAVX512.td:3541-3542
> @@ +3540,4 @@
> +                              X86VectorVTInfo VTI, SDPatternOperator 
> + OpNode> {  defm v213r : avx512_fma3_round_rrb<opc213, !strconcat(OpcodeStr, "213", VTI.Suffix),
> +                              VTI, OpNode>, EVEX_CD8<VTI.EltSize, 
> + CD8VF>;
> +
> ----------------
> I think that the EVEX_CD8 thing can be written as VTI.CD8TupleForm.
> 
> Same later.
> 
> http://reviews.llvm.org/D6835
> 
> EMAIL PREFERENCES
>  http://reviews.llvm.org/settings/panel/emailpreferences/
> 
> 
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150107/0a901605/attachment.html>