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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:19285
     }
+    case Intrinsic::aarch64_sme_get_pstatesm: {
+      SDLoc DL(N);
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127957