[PATCH] D70253: [AArch64][SVE2] Implement remaining SVE2 floating-point intrinsics

Mon Dec 2 09:40:42 PST 2019

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

Thanks @kmclaughlin , LGTM.

================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:898
+                 llvm_i32_ty],
+                [IntrNoMem]>;
+
----------------
kmclaughlin wrote:
> sdesmalen wrote:
> > efriedma wrote:
> > > kmclaughlin wrote:
> > > > sdesmalen wrote:
> > > > > I'd expect the `llvm_i32_ty` to be an immediate for these instructions, right? If so you'll need to add `ImmArg<OpNo>`  to the list of properties.
> > > > > 
> > > > Thanks for taking a look at this :) I tried your suggestion of adding ImmAr<Op> to the list of properties here but had some problems with it (i.e. Cannot select: intrinsic %llvm.aarch64.sve.fmlalb.lane). I don't think this is too much of an issue here as we have additional checks on the immediate with VectorIndexH32b, which ensures the immediate is in the correct range.
> > > The point of immarg markings isn't to assist the backend; it's to ensure IR optimizations don't break your intrinsic calls.
> > The pattern is probably not matching because the immediate operand is a `TargetConstant` where the `AsmVectorIndexOpnd` derives from `ImmLeaf`, rather than `TImmLeaf` as introduced by D58232.
> Thanks for the suggestion, this was the reason why the patterns were not matching! As this also affects many of the existing intrinsics not added here or in D70437, I would prefer to address this fully in a separate patch - do you have objections to this?
Okay, I'm happy with you want to make that change in a separate patch. It will also be needed for several of the other SVE intrinsics.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70253