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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
