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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127957