[PATCH] D158626: [AArch64] Add missing vrnd intrinsics

[Dave Green via Phabricator via cfe-commits]

dmgreen added a comment.

Other than where the pattern gets defined this looks OK to me.



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:6309
+
+  def : Pat<(v1f64 (OpNode (v1f64 FPR64:$Rn))),
+           (!cast<Instruction>(NAME # Dr) FPR64:$Rn)>;
----------------
miyengar wrote:
> dmgreen wrote:
> > I think the instructions in this multiclass are the vector variants. Can the pattern be moved to the FRIntNNT/SingleOperandFPNo16 class?
> Thanks for the comment. I have tried to do this, but have run into a problem:
> 
> This pattern targets the LLVM internal intrinsic: `int_aarch64_neon_frint***_v1f64`, as referenced in `AArch64InstrInfo.td`: 
> 
> ```
> defm FRINT32Z : FRIntNNTVector<0, 0, "frint32z", int_aarch64_neon_frint32z>;
> ```
> It pattern matches 'v1f64' to 'Dr', which corresponds to a scalar instruction defined in FRIntNNT/SingleOperandFPNo16 - I.e. it is matching the vector variant to the scalar variant.
> 
> Moving the pattern to FRIntNNT results in an error "Cannot select: intrinsic %llvm.aarch64.neon.frint64x" as FRIntNNT is designed to match the intrinsics whose name does not contain "neon". Since the current patch deals specifically with the neon variant, I don't see an uncomplicated way to modify FRIntNNT to accommodate both variants of such intrinsic (neon and vanilla).
> 
> I have moved the pattern from SIMDTwoVectorSD to FRIntNNTVector though which is hopefully a better place to put it.
> 
> Does this seem like a sensible solution? Thanks!
I see, Because there is a difference between int_aarch64_frint32z and int_aarch64_neon_frint32z.

Could you pass int_aarch64_neon_frint32z to FRIntNNT too, or make them free patterns in AArch64InstrInfo.td? Otherwise this is using the class to generate vector variants of FRINT32Z to also generate the scalar FRINT32ZDr instruction too. It likely doesn't matter a huge amount, but it feels like it breaks the encapsulation of the FRIntNNTVector class.


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

https://reviews.llvm.org/D158626