[PATCH] D132384: [AArch64][PAC] Select MOVK for ptrauth.blend intrinsic.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

I had just 2 more minor nitpicks - mostly about adding 2 more regression tests to test/document expected behavior in corner cases.



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:8410
+
+let Predicates = [HasPAuth] in {
+def : Pat<(int_ptrauth_blend GPR64:$Rd, imm64_0_65535:$imm),
----------------
It seems that the blend intrinsic can be implemented on cores that do not implement the PAuth extension. Which made me wonder if it should be allowed or not for a user to use the blend intrinsic when targeting a core not supporting PAuth?
I don't have a very strong argument either way. I wonder if you happen to have an opinion?
One thing I did think off - but couldn't answer from just looking at this patch - are DAGISel and GISel consistent in rejecting the blend intrinsic when PAuth is not implemented?
I think it would make sense to have a regression test checking that the blend intrinsic is indeed accepted/rejected as expected when targeting a core that does not support PAuth.


================
Comment at: llvm/test/CodeGen/AArch64/ptrauth-intrinsic-blend.ll:19
+; CHECK-NEXT:    ret
+  %tmp = call i64 @llvm.ptrauth.blend(i64 %arg, i64 12345)
+  ret i64 %tmp
----------------
ab wrote:
> kristof.beyls wrote:
> > Overall, I think this patch looks fine.
> > I notice that the definition of the llvm.ptrauth.blend intrinisic is (per https://llvm.org/docs/PointerAuth.html#llvm-ptrauth-blend):
> > 
> > > The integer discriminator argument is a small integer, as specified by the target.
> > and the signature of the llvm.ptrauth.blend is documented as:
> > ```
> > declare i64 @llvm.ptrauth.blend(i64 <address discriminator>, i64 <integer discriminator>)
> > ```
> > 
> > It makes me wonder what should happen when an immediate too large to fit into an i16 is passed....
> > I'm guessing with the patch as currently written, it would result in the backend failing to do instruction selection?
> > Or rather DAGISel failing to do instruction selection and GlobalISel succeeding, taking the lower 16 bits of the immediate?
> > 
> > I wonder if for consistency reasons it would be easily possible to make DAGISel behave like GlobalISel and accept immediates larger than 16 bits, and take the lower 16 bit value?
> > That would be behavior that presumably is consistent also with when the blend value comes from a register rather than an immediate?
> > 
> > 
> Interesting!  I would go the other way, and consider the GISel behavior too lax.  I added a check that makes it fail isel as well.  It's true we don't specify the intrinsics strictly enough, but for AArch64 I'd consider it always incorrect to use a wider value.
> 
> It's also true that that's the emergent behavior for non-immediate blends, but I'd argue those are by far the exception:  I think the only legitimate use is a dynamic loader that really does get the discriminator values from memory.  Anything else is a red flag (and I considered somehow hiding them in the compiler, or detecting accidental uses, but that's tricky.)
> 
> How does that sound?
That sound good to me!
Shouldn't there also be a regression test that verifies that instruction selection fails for an immediate that does not fit in 16 bits?


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

https://reviews.llvm.org/D132384