This is an archive of the discontinued LLVM Phabricator instance.

[X86] Adjust Keylocker store register num for encodekey128/256
ClosedPublic

Authored by xiangzhangllvm on Sep 9 2021, 12:34 AM.

Details

Summary

We plan let encodekey128/256 not store "0" value xmm* regs into handle mem.

XMM0 -> Handle[127:0]; // AAD
XMM1 -> Handle[255:128]; // Integrity Tag
XMM2 -> Handle[383:256]; // CipherText
XMM4 := 0; // Reserved for future usage
XMM5 := 0; // Reserved for future usage
XMM6 := 0; // Reserved for future usage

Diff Detail

Event Timeline

xiangzhangllvm created this revision.Sep 9 2021, 12:34 AM
xiangzhangllvm requested review of this revision.Sep 9 2021, 12:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2021, 12:34 AM
xiangzhangllvm planned changes to this revision.Sep 9 2021, 12:40 AM

Draft

xiangzhangllvm requested review of this revision.Sep 9 2021, 2:21 AM
xiangzhangllvm edited the summary of this revision. (Show Details)

These stores aren’t going to handle memory. The encode instruction read from handle memory. The stores are to an array provided by the caller of the intrinsic that only exists because we can’t return multiple values in C. If you don’t store them then those values in the user array are unitialized.

These stores aren’t going to handle memory. The encode instruction read from handle memory. The stores are to an array provided by the caller of the intrinsic that only exists because we can’t return multiple values in C. If you don’t store them then those values in the user array are unitialized.

The encodekey instruction generate key handler. User expect the buffer size of handler is 3*16 bytes for _mm_encodekey128_u32 and 4*16 for _mm_encodekey2568_u32. Storing extra data to the buffer either need extra instruction or need extra memory space for user.

LuoYuanke added a comment.EditedSep 9 2021, 8:11 AM

Here (https://godbolt.org/z/EzPcsnz6f) is the difference codegen for clang and gcc. xmm4~xmm6 are zero value and are not part of key handler.

You need to update the doxygen comment in the intrinsic header too.

The pseudocode for _mm256_encodekey256_u32 is incorrect on the Intel Intrinsics Guide

__h[383:0] := WrapKey256(__key_lo[127:0], __key_hi[127:0], __htype)
dst[0] := IWKey.NoBackup
dst[4:1] := IWKey.KeySource[3:0]

That should be __h[511:0]

Yes, For encodekey256, That should be __h[511:0] , thanks!

Yes, For encodekey256, That should be __h[511:0] , thanks!

Is there a real place to file Intrinsic Guide issues? The "Questions? Issues?" takes you here https://community.intel.com/t5/Software/ct-p/software-products/topic/363747 but doesn't look like an obvious place to make sure the right people see it.

+ @FreddyYe , do you know Craig's question ?

Yes, For encodekey256, That should be __h[511:0] , thanks!

Is there a real place to file Intrinsic Guide issues? The "Questions? Issues?" takes you here https://community.intel.com/t5/Software/ct-p/software-products/topic/363747 but doesn't look like an obvious place to make sure the right people see it.

Yes, we haven't found a good place for reporting bugs. Will try to fix the problem. Currently, @FreddyYe and I will handle it internally if we notice the intrinsics guide issues.

I am appling the access to modify the intrinsic guide, it may need more than 24h.

Could we let this patch in first? They are 2 independent jobs.

As Craig mentions we need to change the comments of header file keylockerintrin.h. For example change ((__m128i*)__h) to ((__m128i*)__h) + 5 to ((__m128i*)__h) to ((__m128i*)__h) + 2.

As Craig mentions we need to change the comments of header file keylockerintrin.h. For example change ((__m128i*)__h) to ((__m128i*)__h) + 5 to ((__m128i*)__h) to ((__m128i*)__h) + 2.

Also

/ MEM[h+511:h+384] := 0 Reserved for future usage

124 /// MEM[__h+639:__h+512] := 0 // Reserved for future usage
125 /// MEM[__h+767:__h+640] := 0 // Reserved for future usage

Update related comments for intrinsic API.

This revision is now accepted and ready to land.Sep 12 2021, 11:45 PM
This revision was landed with ongoing or failed builds.Sep 13 2021, 3:04 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2021, 3:04 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript