[llvm-commits] Please review: FMA implementation for X86 architecture

Craig Topper craig.topper at gmail.com
Tue Jul 31 21:48:05 PDT 2012 

Please review the patch for tab characters and trailing whitespace.

   defm VFMADDPD    : fma3p_forms<0x98, 0xA8, 0xB8, "vfmadd", "pd",
memopv2f64,
-    memopv4f64, int_x86_fma_vfmadd_pd, int_x86_fma_vfmadd_pd_256>, VEX_W;
+    memopv4f64, int_x86_fma_vfmadd_pd, int_x86_fma_vfmadd_pd_256,
X86Fmadd, v2f64, v4f64>, VEX_W;
   defm VFMSUBPD    : fma3p_forms<0x9A, 0xAA, 0xBA, "vfmsub", "pd",
memopv2f64,
-    memopv4f64, int_x86_fma_vfmsub_pd, int_x86_fma_vfmsub_pd_256>, VEX_W;
+    memopv4f64, int_x86_fma_vfmsub_pd, int_x86_fma_vfmsub_pd_256,
X86Fmsub, v2f64, v4f64>, VEX_W;
   defm VFMADDSUBPD : fma3p_forms<0x96, 0xA6, 0xB6, "vfmaddsub", "pd",
memopv2f64,
-    memopv4f64, int_x86_fma_vfmaddsub_pd, int_x86_fma_vfmaddsub_pd_256>,
VEX_W;
+    memopv4f64, int_x86_fma_vfmaddsub_pd, int_x86_fma_vfmaddsub_pd_256,
X86Fmaddsub, v2f64, v4f64>, VEX_W;
   defm VFMSUBADDPD : fma3p_forms<0x97, 0xA7, 0xB7, "vfmsubadd", "pd",
memopv2f64,
-    memopv4f64, int_x86_fma_vfmsubadd_pd, int_x86_fma_vfmsubadd_pd_256>,
VEX_W;
+    memopv4f64, int_x86_fma_vfmsubadd_pd, int_x86_fma_vfmsubadd_pd_256,
X86Fmsubadd, v2f64, v4f64>, VEX_W;

Please keep lines below 80 columns. This is just an example. There are
others.

+  if (ScalarVT != MVT::f32 && ScalarVT != MVT::f64 || !Subtarget->hasFMA())
+    return SDValue();

This code fails -Wlogical-op-parentheses. Please put parentheses around the
&& part.

      setOperationAction(ISD::FMA,             MVT::v8f32, Custom);
      setOperationAction(ISD::FMA,             MVT::v4f64, Custom);
      setOperationAction(ISD::FMA,             MVT::v4f32, Custom);
      setOperationAction(ISD::FMA,             MVT::v2f64, Custom);
      setOperationAction(ISD::FMA,             MVT::f32, Custom);
      setOperationAction(ISD::FMA,             MVT::f64, Custom);

Why are these under hasAVX2() instead of hasFMA()?

+static SDValue PerformFMACombine(SDNode *N, SelectionDAG &DAG,
+                                 const X86Subtarget* Subtarget) {
+  DebugLoc dl = N->getDebugLoc();
+  SDValue N0 = N->getOperand(0);
+  EVT VT = N->getValueType(0);

The N0 variable here is unused.

Other than those thing it looks good to me.


On Thu, Jul 26, 2012 at 12:52 AM, Demikhovsky, Elena <
elena.demikhovsky at intel.com> wrote:

>  Thank you, Lang.****
>
> ** **
>
> Craig did not answer yet. I’ll wait a few days.****
>
> ** **
>
> ** **
>
> *- Elena*****
>
> *From:* Lang Hames [mailto:lhames at gmail.com]
> *Sent:* Wednesday, July 25, 2012 21:45
> *To:* Demikhovsky, Elena
> *Cc:* Craig Topper; Commit Messages and Patches for LLVM
>
> *Subject:* Re: [llvm-commits] Please review: FMA implementation for X86
> architecture****
>
>  ** **
>
> Hi Elena,****
>
> ** **
>
> You might want to check with Craig about the X86 specifics, since I'm not
> very familiar with the X86 FMA work, but the DAG combines look good.****
>
> ** **
>
> One minor style note:****
>
> ** **
>
> +  // Negative multiplication when NegA xor NegB ****
>
> +  bool NegMul = (NegA || NegB) && !(NegA && NegB); ****
>
> ** **
>
> This could be expressed as ****
>
> ** **
>
> bool NegMul = NegA ^ NegB;****
>
> or****
>
> bool NegMul = NegA != NegB;****
>
> ** **
>
> Otherwise it looks good to me.****
>
> ** **
>
> Cheers,****
>
> Lang.****
>
> ** **
>
> On Wed, Jul 25, 2012 at 2:36 AM, Demikhovsky, Elena <
> elena.demikhovsky at intel.com> wrote:****
>
> I’d like to commit this patch.****
>
> Hi Craig,****
>
>  ****
>
> Any objections?****
>
>  ****
>
> Thanks****
>
>  ****
>
> *- Elena*****
>
> *From:* Demikhovsky, Elena
> *Sent:* Tuesday, July 24, 2012 09:51
> *To:* 'Lang Hames'
> *Subject:* RE: [llvm-commits] Please review: FMA implementation for X86
> architecture****
>
>  ****
>
> Hi Lang,****
>
>  ****
>
> You are right. The expression x--(y*z) was converted to x+(y*z) before it
> reaches Combine().****
>
> I removed the code bellow.****
>
>  ****
>
> May I commit? ****
>
>  ****
>
> Thanks****
>
>  ****
>
> *- Elena*****
>
> *From:* Lang Hames [mailto:lhames at gmail.com <lhames at gmail.com>]
> *Sent:* Tuesday, July 24, 2012 00:19
> *To:* Demikhovsky, Elena
> *Cc:* Commit Messages and Patches for LLVM
> *Subject:* Re: [llvm-commits] Please review: FMA implementation for X86
> architecture****
>
>  ****
>
> Hi Elena,****
>
>  ****
>
> The second DAG combine looks a bit off:****
>
>  ****
>
> +    // fold (fsub x, -(fmul, y, z)) -> (fma (fneg y), z, x)****
>
>  ****
>
> That implies:****
>
>  ****
>
> x - -(y * z) ==> -y * z + a****
>
>  ****
>
> Which isn't a valid transformation. You probably don't want the negate on
> 'y'.****
>
>  ****
>
> +    if (N1.getOpcode() == ISD::FNEG && ****
>
> +        N1.getOperand(0).getOpcode() == ISD::FMUL &&****
>
> +        N1->hasOneUse() && N1.getOperand(0).hasOneUse()) {****
>
> +      SDValue N10 = N1.getOperand(0).getOperand(0);****
>
> +      SDValue N11 = N1.getOperand(0).getOperand(1);****
>
> +      return DAG.getNode(ISD::FMA, dl, VT,****
>
> +                         DAG.getNode(ISD::FNEG, N1->getDebugLoc(), VT,
> N10),****
>
> +                         N11, N1);****
>
>  ****
>
> Given the observation above, I think the ISD::FNEG should go away here.
> Also N1 is being used as the RHS addend. I think that should be N0?****
>
>  ****
>
> Otherwise it looks good to me.****
>
>  ****
>
> Cheers,****
>
> Lang.****
>
>  ****
>
>  ****
>
> On Mon, Jul 23, 2012 at 8:43 AM, Demikhovsky, Elena <
> elena.demikhovsky at intel.com> wrote:****
>
>
> I added code generation for FMA patterns on AVX2 architecture.
>
> - Elena
>
>
> ---------------------------------------------------------------------
> 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
> 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.****
>
> ** **
>  ---------------------------------------------------------------------
> 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.
>



-- 
~Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120731/5adafc15/attachment.html>

More information about the llvm-commits mailing list