This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][PAC] Select MOVK for ptrauth.blend intrinsic.
ClosedPublic

Authored by ab on Aug 22 2022, 8:56 AM.

Details

Summary

In a sense, this is implementing an ABI decision (how to lower the software construct of "blend"), but if there are interesting variants to consider, this could be made object-file-format-specific in some way.

Diff Detail

Event Timeline

ab created this revision.Aug 22 2022, 8:56 AM
Herald added a project: Restricted Project. · View Herald Transcript
ab requested review of this revision.Aug 22 2022, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2022, 8:56 AM
kristof.beyls added inline comments.Aug 23 2022, 8:46 AM
llvm/test/CodeGen/AArch64/ptrauth-intrinsic-blend.ll
20

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?

ab updated this revision to Diff 456775.Aug 30 2022, 1:27 PM
  • further constrain GISel immediate blends to fail if wider than 16 bits
ab added inline comments.Aug 30 2022, 1:29 PM
llvm/test/CodeGen/AArch64/ptrauth-intrinsic-blend.ll
20

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?

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

llvm/lib/Target/AArch64/AArch64InstrInfo.td
8410

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.

llvm/test/CodeGen/AArch64/ptrauth-intrinsic-blend.ll
20

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?

ab updated this revision to Diff 470169.Oct 24 2022, 8:34 AM
ab added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.td
8410

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 strong argument either, but until we have a use for it on other cores, I'd leave it as disallowed there?
Somewhat related: XPAC/strip is, however, commonly needed on cores that don't support PAC (or aren't even aarch64), usually for debugging and crash analysis tools. But that one actually depends on aarch64 processor state (TBI, T0Sz), sadly.

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?

They're not, and I found out by later tests breaking ;) On SDAG, if we can't match the 16-bit immediate, we'll fallback to the arbitrary GPR64 value (which isn't ideal by itself.) So we can't easily fail on wider values, at least not without some machinery to match this case (maybe matching any immediate and erroring out when trying to encode; I'm not sure we can fail in a good way at isel time)

kristof.beyls accepted this revision.Nov 22 2022, 6:30 AM
kristof.beyls added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.td
8410

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 strong argument either, but until we have a use for it on other cores, I'd leave it as disallowed there?

That seems OK to me.

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?

They're not, and I found out by later tests breaking ;) On SDAG, if we can't match the 16-bit immediate, we'll fallback to the arbitrary GPR64 value (which isn't ideal by itself.) So we can't easily fail on wider values, at least not without some machinery to match this case (maybe matching any immediate and erroring out when trying to encode; I'm not sure we can fail in a good way at isel time)

I also don't see an immediate solution to this.
It seems to me that we shouldn't hold up progress on the upstreaming of this functionality on this minor issue. In other words, I think this issue can be ignored, at least for now.

This revision is now accepted and ready to land.Nov 22 2022, 6:30 AM

Could this patch be landed? (I can commit it on your behalf if appropriate)

Overall, I would like to assist in preparing for review and upstreaming more PAC-related patches from https://github.com/ahmedbougacha/llvm-project/commits/eng/arm64e-upstream-llvmorg .

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
5878–5879

[nitpick] Maybe reorder these lines so the first removeOperand doesn't affect the index passed to the second one

I think this patch can be landed. @ab, do you see any reason not to land this patch?

I am planning to land this patch the next week, if there will be no objections.

ab added a comment.Jun 10 2023, 3:42 PM

I am planning to land this patch the next week, if there will be no objections.

I don’t think that’s how we do things. Either way I was out for a while and will update the patches; thanks for taking a look.

This revision was landed with ongoing or failed builds.Jun 26 2023, 9:44 AM
This revision was automatically updated to reflect the committed changes.