[PATCH] D82483: [SVE] Code generation for fixed length vector adds.

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally accepted this revision.
cameron.mcinally added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14897
+    Operands.push_back(V);
+  }
+
----------------
paulwalker-arm wrote:
> cameron.mcinally wrote:
> > Could we modify the assert to remove the branch?
> > 
> > ```
> > assert(isa<CondCodeSDNode>(V) || V.getValueType().isScalableVector() &&
> >            "Only scalable vectors are supported!");
> > ```
> > 
> > Same with the code above too.
> I can modify this case but the earlier code is not a straight copy so some kind of control flow is required. 
Oh, I missed that. If you want to leave it so the two cases are structured similarly, that's fine.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:1618
+  def : SVE_3_Op_Pat_SelZero<nxv2f64, int_op, nxv2i1, nxv2f64, nxv2f64, !cast<Pseudo>(NAME # _ZERO_D)>;
 }
 
----------------
paulwalker-arm wrote:
> cameron.mcinally wrote:
> > Is having both ir_op and int_op problematic going forward? E.g. how to match an intrinsic with an undef merge.
> > 
> > X86 solves a similar problem with the tables in `lib/Target/X86/X86IntrinsicsInfo.h`. That might be something to consider long term. I'm not sure if it's a great fit though.
> > 
> > I don't think this needs to be changed now, but something to consider...
> > 
> I don't believe there's anything problematic as there's nothing to prevent adding SVE_3_Op_Pat_SelZero patterns for ir_op and SVE_3_Op_Pat for int_op.  It's just that this patch does not have tests for those cases so I've not added the patterns.
Good point. I'm not sure if canonicalizing the intrinsics would save code or add more, so I'll leave it there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82483/new/

https://reviews.llvm.org/D82483