[PATCH] D71483: [AArch64][SVE] Add patterns for logical immediate operations.

[Danilo Carvalho Grael via Phabricator via llvm-commits]

dancgr marked an inline comment as done.
dancgr added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:1064
                 [LLVMMatchType<0>,
-                 llvm_i32_ty],
+                 llvm_anyint_ty],
                 [IntrNoMem, ImmArg<1>]>;
----------------
dancgr wrote:
> efriedma wrote:
> > dancgr wrote:
> > > efriedma wrote:
> > > > I don't really like anyint here... can we restrict the type somehow?
> > > > 
> > > > Actually, I'm not sure how this even works with your testcases; how is LLVM computing the type of llvm.aarch64.sve.orr.imm.nxv16i8?  You aren't specifying the type of the integer.
> > > We can split this into two intrinsics, one i32 and one i64, both will be almost exactly the same with only the immediate type difference. I don't see a problem in that, I was just trying to reuse some code to avoid unnecessary complexity. But I'm not opposed to having two separate ones either.
> > > 
> > > In the following test case I'm explicitly setting the input as i64 on the declare statement. If I were to put i32 there, It would fail to find a matching pattern.
> > > 
> > > ```
> > > define <vscale x 16 x i8> @orr_i8(<vscale x 16 x i8> %a) {
> > > ; CHECK-LABEL: orr_i8:
> > > ; CHECK: orr z0.b, z0.b, #0xf
> > > ; CHECK-NEXT: ret
> > >   %res = call <vscale x 16 x i8> @llvm.aarch64.sve.orr.imm.nxv16i8(<vscale x 16 x i8> %a, 
> > >                                                                    i64 15)
> > >   ret <vscale x 16 x i8> %res
> > > }
> > > 
> > > declare <vscale x 16 x i8> @llvm.aarch64.sve.orr.imm.nxv16i8(<vscale x 16 x i8>, i64)
> > > ```
> > Why do we need both an i32 and an i64 variant?  The i64 variant seems to cover all the relevant cases (and you haven't added any tests for the i32 variant).
> > 
> > I figured out the source of my confusion about the names of the intrinsics; apparently IR autoupgrade is "fixing" the name of llvm.aarch64.sve.orr.imm.nxv16i8 to refer to the actual name, llvm.aarch64.sve.orr.imm.nxv16i8.i64.
> Oh, I get it now. The AdvSIMD_1VectorArg_Imm_Intrinsic was introduced previously for the add/sub/sqadd imm. instrinsics (which use i32). And then I changed it to anyint to be able to use the same class for and/orr/eor imm., because those use i64.
But I don't mind changing that. I am just trying to understand the reasoning so I can apply it to my future patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71483