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

[Adam Nemet]

> On Jan 7, 2015, at 11:42 AM, Demikhovsky, Elena <elena.demikhovsky at intel.com> wrote:
> 
> > 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.
> It may cause FP exception.

In those cases I am not convinced that we are correct in generating a vselect.

> > In this case I am not convinced that we can’t generate the wrong result.
> If ROUNDMODE and instruction will be separated somehow, the compilation will fail.
> I say “somehow” because we are talking about intrinsic that mapped directly to instruction - everything is legal inside.
> Can you give an example?
> The alternative is to invent one more node per instruction that needs rounding mode.

Sounds like we may want to create AVX512-specific nodes for all FP ops.  Then we could have both the mask register and the rounding mode as operands.

>  
> > 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.)
> I see this as a small patch which resolves FP intrinsics rounding mode, but no problem, we can widen the list of reviewers.

It’s usually not the size of patch that matters but whether it’s changing the design.  In this case, I think that we’re breaking the notion that the SDAG (A (B)) is legal if both A and B have ISA counterparts (whether (A (B)) together has a match is secondary).  As you say we’re late in the backend so this could made an exception but this still feels like an extension of the design.

Adam

>  
> -           Elena
>  
> From: Adam Nemet [mailto:anemet at apple.com <mailto:anemet at apple.com>] 
> Sent: Wednesday, January 07, 2015 20:09
> To: Demikhovsky, Elena
> Cc: reviews+D6835+public+db07cccdafa4527c at reviews.llvm.org <mailto:reviews+D6835+public+db07cccdafa4527c at reviews.llvm.org>; Badouh, Asaf; llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> Subject: Re: [PATCH] [AVX-512] - Add FMA instruction with Rounding mode
>  
>  
> On Jan 6, 2015, at 11:12 PM, Demikhovsky, Elena <elena.demikhovsky at intel.com <mailto: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 <http://reviews.llvm.org/D6835>
> 
> EMAIL PREFERENCES
>  http://reviews.llvm.org/settings/panel/emailpreferences/ <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>
>  
> ---------------------------------------------------------------------
> 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.
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150107/18d5a8b4/attachment.html>