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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab added inline comments.


================
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
----------------
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?


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

https://reviews.llvm.org/D132384