[PATCH] D127957: [AArch64][SME] Add some SME PSTATE setting/query intrinsics

[David Sherwood via Phabricator via llvm-commits]

david-arm marked 3 inline comments as done.
david-arm added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:2672-2677
+  def int_aarch64_sme_get_tpidr2
+      : DefaultAttrsIntrinsic<[llvm_i64_ty], [],
+                              [IntrNoMem, IntrHasSideEffects]>;
+  def int_aarch64_sme_set_tpidr2
+      : DefaultAttrsIntrinsic<[], [llvm_i64_ty],
+                              [IntrNoMem, IntrHasSideEffects]>;
----------------
c-rhodes wrote:
> can we lower to the generic `llvm.read/write_register` intrinsics for TPIDR2_EL0?
I had a look and this doesn't seem to be something used very often to be honest. It has quite a cumbersome interface because you have to pass in metadata for the first argument specifying the register and I worry it makes it more difficult if we ever want to look for read/writes of TPIDR2_EL0. I feel the approach we have in this patch is at least consistent with all the other AArch64 intrinsics that read/write system registers, e.g. int_aarch64_sve_rdffr, int_aarch64_sve_wrffr, etc.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:19285
     }
+    case Intrinsic::aarch64_sme_get_pstatesm: {
+      SDLoc DL(N);
----------------
david-arm wrote:
> aemerson wrote:
> > david-arm wrote:
> > > aemerson wrote:
> > > > This is a lowering but it’s being implemented in a combine method, which although it may work, doesn’t seem the right place to do it.
> > > So you're absolutely right @aemerson, it doesn't feel like the right place to do it. We wanted the intrinsic to return a boolean i1 value, but the problem is that when you do that there is a promotion error:
> > > 
> > >   PromoteIntegerResult #0: t2: i1,ch = llvm.aarch64.sme.get.pstatesm t0, TargetConstant:i64<644>
> > > 
> > > and it's not easy to fix.
> > > 
> > > So there are at least two simple solutions:
> > > 
> > > 1. Lower this in a DAG combine, which is a fairly maverick thing to do I know. :)
> > > 2. Change the intrinsic to return a i32 so we don't have to worry about promotion.
> > > 
> > > We chose one because we thought a i1 return made sense, but I'm happy to consider 2 if you prefer it!
> > Can't this be done in  `AArch64TargetLowering::LowerINTRINSIC_WO_CHAIN()`? I would have thought if you do a lowering into custom nodes then you bypass any legalization issues with intrinsics. Even if you do still run into that issue, changing it to return `i32` seems reasonable to me.
> So there is still a problem here and it looks like Intrinsic::aarch64_rndr is a similar example where someone also had to lower this using performDAGCombine. Even if I make aarch64_sme_get_pstatesm return a i64 value we still can't lower this normally in AArch64TargetLowering::LowerINTRINSIC_WO_CHAIN. The problem is that the AArch64ISD::MRS node needs a chain, which we have during DAG combine. However, in AArch64TargetLowering::LowerINTRINSIC_WO_CHAIN we don't have the chain - I could potentially try to create one, but at least it seems like for Intrinsic::aarch64_rndr someone decided not to do that
So it needs a chain because the MRS node needs a chain. It was a bit fiddly trying to get this to work, but eventually discovered you have to mark INTRINSIC_W_CHAIN for MVT::Other as needing custom lowering.


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

https://reviews.llvm.org/D127957